This patch series cleans up the DDC code a little bit so that
it is more efficient time wise and supports grabbing the EDID
of the eDP panel over the aux channel. I timed this on a board
I have on my desk and it takes about 20ms to grab the EDID out
of the panel and make sure it is valid.
The first two patches seem less controversial so I stuck them at
the beginning. The third patch does the EDID reading and caches
it so we don't have to keep grabbing it over and over again. And
finally the last patch updates the reply field so that short
reads and nacks over the channel are reflected properly instead of
treating them as some sort of error that can't be discerned.
Stephen Boyd (4):
drm/bridge: ti-sn65dsi86: Combine register accesses in
ti_sn_aux_transfer()
drm/bridge: ti-sn65dsi86: Make polling a busy loop
drm/bridge: ti-sn65dsi86: Read EDID blob over DDC
drm/bridge: ti-sn65dsi86: Update reply on aux failures
drivers/gpu/drm/bridge/ti-sn65dsi86.c | 103 ++++++++++++++++++--------
1 file changed, 71 insertions(+), 32 deletions(-)
Cc: Douglas Anderson <[email protected]>
Cc: Laurent Pinchart <[email protected]>
Cc: Jonas Karlman <[email protected]>
Cc: Jernej Skrabec <[email protected]>
Cc: Sean Paul <[email protected]>
base-commit: 3650b228f83adda7e5ee532e2b90429c03f7b9ec
--
Sent by a computer, using git, on the internet
Use the DDC connection to read the EDID from the eDP panel instead of
relying on the panel to tell us the modes.
Reviewed-by: Douglas Anderson <[email protected]>
Reviewed-by: Laurent Pinchart <[email protected]>
Cc: Jonas Karlman <[email protected]>
Cc: Jernej Skrabec <[email protected]>
Cc: Sean Paul <[email protected]>
Acked-by: Sam Ravnborg <[email protected]>
Signed-off-by: Stephen Boyd <[email protected]>
---
drivers/gpu/drm/bridge/ti-sn65dsi86.c | 20 ++++++++++++++++++++
1 file changed, 20 insertions(+)
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index 8276fa50138f..6b6e98ca2881 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -119,6 +119,7 @@
* @debugfs: Used for managing our debugfs.
* @host_node: Remote DSI node.
* @dsi: Our MIPI DSI source.
+ * @edid: Detected EDID of eDP panel.
* @refclk: Our reference clock.
* @panel: Our panel.
* @enable_gpio: The GPIO we toggle to enable the bridge.
@@ -144,6 +145,7 @@ struct ti_sn_bridge {
struct drm_bridge bridge;
struct drm_connector connector;
struct dentry *debugfs;
+ struct edid *edid;
struct device_node *host_node;
struct mipi_dsi_device *dsi;
struct clk *refclk;
@@ -265,6 +267,23 @@ connector_to_ti_sn_bridge(struct drm_connector *connector)
static int ti_sn_bridge_connector_get_modes(struct drm_connector *connector)
{
struct ti_sn_bridge *pdata = connector_to_ti_sn_bridge(connector);
+ struct edid *edid = pdata->edid;
+ int num, 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) {
+ num = drm_add_edid_modes(connector, edid);
+ if (num)
+ return num;
+ }
+ }
return drm_panel_get_modes(pdata->panel, connector);
}
@@ -1245,6 +1264,7 @@ 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);
--
Sent by a computer, using git, on the internet
There's no reason we need to wait here to poll a register over i2c. The
i2c bus is inherently slow and delays are practically part of the
protocol because we have to wait for the device to respond to any
request for a register. Let's rely on the sleeping of the i2c controller
instead of adding any sort of delay here in the bridge driver.
Reviewed-by: Douglas Anderson <[email protected]>
Cc: Laurent Pinchart <[email protected]>
Cc: Jonas Karlman <[email protected]>
Cc: Jernej Skrabec <[email protected]>
Cc: Sean Paul <[email protected]>
Acked-by: Sam Ravnborg <[email protected]>
Signed-off-by: Stephen Boyd <[email protected]>
---
drivers/gpu/drm/bridge/ti-sn65dsi86.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index 87726b9e446f..8276fa50138f 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -881,9 +881,9 @@ static ssize_t ti_sn_aux_transfer(struct drm_dp_aux *aux,
regmap_write(pdata->regmap, SN_AUX_CMD_REG, request_val | AUX_CMD_SEND);
+ /* Zero delay loop because i2c transactions are slow already */
ret = regmap_read_poll_timeout(pdata->regmap, SN_AUX_CMD_REG, val,
- !(val & AUX_CMD_SEND), 200,
- 50 * 1000);
+ !(val & AUX_CMD_SEND), 0, 50 * 1000);
if (ret)
return ret;
--
Sent by a computer, using git, on the internet
We should be setting the drm_dp_aux_msg::reply field if a NACK or a
SHORT reply happens. Update the error bit handling logic in
ti_sn_aux_transfer() to handle these cases and notify upper layers that
such errors have happened. This helps the retry logic understand that a
timeout has happened, or to shorten the read length if the panel isn't
able to handle the longest read possible.
Note: I don't have any hardware that exhibits these code paths so this
is written based on reading the datasheet for this bridge and inspecting
the code and how this is called.
Changes in v2:
- Move WRITE_STATUS_UPDATE check from case to assignment
Changes in v2:
- Handle WRITE_STATUS_UPDATE properly
Reviewed-by: Douglas Anderson <[email protected]>
Cc: Laurent Pinchart <[email protected]>
Cc: Jonas Karlman <[email protected]>
Cc: Jernej Skrabec <[email protected]>
Cc: Sean Paul <[email protected]>
Acked-by: Sam Ravnborg <[email protected]>
Signed-off-by: Stephen Boyd <[email protected]>
---
drivers/gpu/drm/bridge/ti-sn65dsi86.c | 35 +++++++++++++++++++++++----
1 file changed, 30 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index 6b6e98ca2881..3a758c706b70 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -861,7 +861,7 @@ static ssize_t ti_sn_aux_transfer(struct drm_dp_aux *aux,
struct drm_dp_aux_msg *msg)
{
struct ti_sn_bridge *pdata = aux_to_ti_sn_bridge(aux);
- u32 request = msg->request & ~DP_AUX_I2C_MOT;
+ u32 request = msg->request & ~(DP_AUX_I2C_MOT | DP_AUX_I2C_WRITE_STATUS_UPDATE);
u32 request_val = AUX_CMD_REQ(msg->request);
u8 *buf = msg->buffer;
unsigned int len = msg->size;
@@ -878,6 +878,8 @@ static ssize_t ti_sn_aux_transfer(struct drm_dp_aux *aux,
case DP_AUX_NATIVE_READ:
case DP_AUX_I2C_READ:
regmap_write(pdata->regmap, SN_AUX_CMD_REG, request_val);
+ /* Assume it's good */
+ msg->reply = 0;
break;
default:
return -EINVAL;
@@ -909,10 +911,33 @@ static ssize_t ti_sn_aux_transfer(struct drm_dp_aux *aux,
ret = regmap_read(pdata->regmap, SN_AUX_CMD_STATUS_REG, &val);
if (ret)
return ret;
- else if ((val & AUX_IRQ_STATUS_NAT_I2C_FAIL)
- || (val & AUX_IRQ_STATUS_AUX_RPLY_TOUT)
- || (val & AUX_IRQ_STATUS_AUX_SHORT))
- return -ENXIO;
+
+ if (val & AUX_IRQ_STATUS_AUX_RPLY_TOUT) {
+ /*
+ * The hardware tried the message seven times per the DP spec
+ * but it hit a timeout. We ignore defers here because they're
+ * handled in hardware.
+ */
+ return -ETIMEDOUT;
+ }
+
+ if (val & AUX_IRQ_STATUS_AUX_SHORT) {
+ ret = regmap_read(pdata->regmap, SN_AUX_LENGTH_REG, &len);
+ if (ret)
+ return ret;
+ } else if (val & AUX_IRQ_STATUS_NAT_I2C_FAIL) {
+ switch (request) {
+ case DP_AUX_I2C_WRITE:
+ case DP_AUX_I2C_READ:
+ msg->reply |= DP_AUX_I2C_REPLY_NACK;
+ break;
+ case DP_AUX_NATIVE_READ:
+ case DP_AUX_NATIVE_WRITE:
+ msg->reply |= DP_AUX_NATIVE_REPLY_NACK;
+ break;
+ }
+ return 0;
+ }
if (request == DP_AUX_NATIVE_WRITE || request == DP_AUX_I2C_WRITE ||
len == 0)
--
Sent by a computer, using git, on the internet
These register reads and writes are sometimes directly next to each
other in the register address space. Let's use regmap bulk read/write
APIs to get the data with one transfer instead of multiple i2c
transfers. This helps cut down on the number of transfers in the case of
something like reading an EDID where we read in blocks of 16 bytes at a
time and the last for loop here is sending an i2c transfer for each of
those 16 bytes, one at a time. Ouch!
Changes in v3:
- Undid changes in v2
Changes in v2:
- Combined AUX_CMD register write
Reviewed-by: Douglas Anderson <[email protected]>
Cc: Laurent Pinchart <[email protected]>
Cc: Jonas Karlman <[email protected]>
Cc: Jernej Skrabec <[email protected]>
Cc: Sean Paul <[email protected]>
Acked-by: Sam Ravnborg <[email protected]>
Signed-off-by: Stephen Boyd <[email protected]>
---
drivers/gpu/drm/bridge/ti-sn65dsi86.c | 50 ++++++++++++---------------
1 file changed, 22 insertions(+), 28 deletions(-)
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index ecdf9b01340f..87726b9e446f 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -17,6 +17,8 @@
#include <linux/regmap.h>
#include <linux/regulator/consumer.h>
+#include <asm/unaligned.h>
+
#include <drm/drm_atomic.h>
#include <drm/drm_atomic_helper.h>
#include <drm/drm_bridge.h>
@@ -72,6 +74,7 @@
#define SN_AUX_ADDR_19_16_REG 0x74
#define SN_AUX_ADDR_15_8_REG 0x75
#define SN_AUX_ADDR_7_0_REG 0x76
+#define SN_AUX_ADDR_MASK GENMASK(19, 0)
#define SN_AUX_LENGTH_REG 0x77
#define SN_AUX_CMD_REG 0x78
#define AUX_CMD_SEND BIT(0)
@@ -841,11 +844,13 @@ static ssize_t ti_sn_aux_transfer(struct drm_dp_aux *aux,
struct ti_sn_bridge *pdata = aux_to_ti_sn_bridge(aux);
u32 request = msg->request & ~DP_AUX_I2C_MOT;
u32 request_val = AUX_CMD_REQ(msg->request);
- u8 *buf = (u8 *)msg->buffer;
+ u8 *buf = msg->buffer;
+ unsigned int len = msg->size;
unsigned int val;
- int ret, i;
+ int ret;
+ u8 addr_len[SN_AUX_LENGTH_REG + 1 - SN_AUX_ADDR_19_16_REG];
- if (msg->size > SN_AUX_MAX_PAYLOAD_BYTES)
+ if (len > SN_AUX_MAX_PAYLOAD_BYTES)
return -EINVAL;
switch (request) {
@@ -859,19 +864,14 @@ static ssize_t ti_sn_aux_transfer(struct drm_dp_aux *aux,
return -EINVAL;
}
- regmap_write(pdata->regmap, SN_AUX_ADDR_19_16_REG,
- (msg->address >> 16) & 0xF);
- regmap_write(pdata->regmap, SN_AUX_ADDR_15_8_REG,
- (msg->address >> 8) & 0xFF);
- regmap_write(pdata->regmap, SN_AUX_ADDR_7_0_REG, msg->address & 0xFF);
-
- regmap_write(pdata->regmap, SN_AUX_LENGTH_REG, msg->size);
+ BUILD_BUG_ON(sizeof(addr_len) != sizeof(__be32));
+ put_unaligned_be32((msg->address & SN_AUX_ADDR_MASK) << 8 | len,
+ addr_len);
+ regmap_bulk_write(pdata->regmap, SN_AUX_ADDR_19_16_REG, addr_len,
+ ARRAY_SIZE(addr_len));
- if (request == DP_AUX_NATIVE_WRITE || request == DP_AUX_I2C_WRITE) {
- for (i = 0; i < msg->size; i++)
- regmap_write(pdata->regmap, SN_AUX_WDATA_REG(i),
- buf[i]);
- }
+ if (request == DP_AUX_NATIVE_WRITE || request == DP_AUX_I2C_WRITE)
+ regmap_bulk_write(pdata->regmap, SN_AUX_WDATA_REG(0), buf, len);
/* Clear old status bits before start so we don't get confused */
regmap_write(pdata->regmap, SN_AUX_CMD_STATUS_REG,
@@ -895,21 +895,15 @@ static ssize_t ti_sn_aux_transfer(struct drm_dp_aux *aux,
|| (val & AUX_IRQ_STATUS_AUX_SHORT))
return -ENXIO;
- if (request == DP_AUX_NATIVE_WRITE || request == DP_AUX_I2C_WRITE)
- return msg->size;
+ if (request == DP_AUX_NATIVE_WRITE || request == DP_AUX_I2C_WRITE ||
+ len == 0)
+ return len;
- for (i = 0; i < msg->size; i++) {
- unsigned int val;
- ret = regmap_read(pdata->regmap, SN_AUX_RDATA_REG(i),
- &val);
- if (ret)
- return ret;
-
- WARN_ON(val & ~0xFF);
- buf[i] = (u8)(val & 0xFF);
- }
+ ret = regmap_bulk_read(pdata->regmap, SN_AUX_RDATA_REG(0), buf, len);
+ if (ret)
+ return ret;
- return msg->size;
+ return len;
}
static int ti_sn_bridge_parse_dsi_host(struct ti_sn_bridge *pdata)
--
Sent by a computer, using git, on the internet
Hi Stephen
On Mon, Nov 02, 2020 at 10:11:40AM -0800, Stephen Boyd wrote:
> This patch series cleans up the DDC code a little bit so that
> it is more efficient time wise and supports grabbing the EDID
> of the eDP panel over the aux channel. I timed this on a board
> I have on my desk and it takes about 20ms to grab the EDID out
> of the panel and make sure it is valid.
>
> The first two patches seem less controversial so I stuck them at
> the beginning. The third patch does the EDID reading and caches
> it so we don't have to keep grabbing it over and over again. And
> finally the last patch updates the reply field so that short
> reads and nacks over the channel are reflected properly instead of
> treating them as some sort of error that can't be discerned.
>
> Stephen Boyd (4):
> drm/bridge: ti-sn65dsi86: Combine register accesses in
> ti_sn_aux_transfer()
> drm/bridge: ti-sn65dsi86: Make polling a busy loop
> drm/bridge: ti-sn65dsi86: Read EDID blob over DDC
> drm/bridge: ti-sn65dsi86: Update reply on aux failures
All applied to drm-misc-next, thanks,
Sam
Hi Stephen,
Reviving a bit of an old thread, for a question.
On Mon, Nov 02, 2020 at 10:11:43AM -0800, Stephen Boyd wrote:
> Use the DDC connection to read the EDID from the eDP panel instead of
> relying on the panel to tell us the modes.
>
> Reviewed-by: Douglas Anderson <[email protected]>
> Reviewed-by: Laurent Pinchart <[email protected]>
> Cc: Jonas Karlman <[email protected]>
> Cc: Jernej Skrabec <[email protected]>
> Cc: Sean Paul <[email protected]>
> Acked-by: Sam Ravnborg <[email protected]>
> Signed-off-by: Stephen Boyd <[email protected]>
> ---
> drivers/gpu/drm/bridge/ti-sn65dsi86.c | 20 ++++++++++++++++++++
> 1 file changed, 20 insertions(+)
>
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index 8276fa50138f..6b6e98ca2881 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -119,6 +119,7 @@
> * @debugfs: Used for managing our debugfs.
> * @host_node: Remote DSI node.
> * @dsi: Our MIPI DSI source.
> + * @edid: Detected EDID of eDP panel.
> * @refclk: Our reference clock.
> * @panel: Our panel.
> * @enable_gpio: The GPIO we toggle to enable the bridge.
> @@ -144,6 +145,7 @@ struct ti_sn_bridge {
> struct drm_bridge bridge;
> struct drm_connector connector;
> struct dentry *debugfs;
> + struct edid *edid;
> struct device_node *host_node;
> struct mipi_dsi_device *dsi;
> struct clk *refclk;
> @@ -265,6 +267,23 @@ connector_to_ti_sn_bridge(struct drm_connector *connector)
> static int ti_sn_bridge_connector_get_modes(struct drm_connector *connector)
> {
> struct ti_sn_bridge *pdata = connector_to_ti_sn_bridge(connector);
> + struct edid *edid = pdata->edid;
> + int num, ret;
> +
> + if (!edid) {
> + pm_runtime_get_sync(pdata->dev);
> + edid = pdata->edid = drm_get_edid(connector, &pdata->aux.ddc);
> + pm_runtime_put(pdata->dev);
Is there any specific reason to use the indirect access method, compared
to the direct method that translates access to an I2C ancillary address
to an I2C-over-AUX transaction (see page 20 of SLLSEH2B) ? The direct
method seems it would be more efficient.
> + }
> +
> + if (edid && drm_edid_is_valid(edid)) {
> + ret = drm_connector_update_edid_property(connector, edid);
> + if (!ret) {
> + num = drm_add_edid_modes(connector, edid);
> + if (num)
> + return num;
> + }
> + }
>
> return drm_panel_get_modes(pdata->panel, connector);
> }
> @@ -1245,6 +1264,7 @@ 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
Quoting Laurent Pinchart (2021-03-17 17:20:43)
> Hi Stephen,
>
> Reviving a bit of an old thread, for a question.
>
> On Mon, Nov 02, 2020 at 10:11:43AM -0800, Stephen Boyd wrote:
> > @@ -265,6 +267,23 @@ connector_to_ti_sn_bridge(struct drm_connector *connector)
> > static int ti_sn_bridge_connector_get_modes(struct drm_connector *connector)
> > {
> > struct ti_sn_bridge *pdata = connector_to_ti_sn_bridge(connector);
> > + struct edid *edid = pdata->edid;
> > + int num, ret;
> > +
> > + if (!edid) {
> > + pm_runtime_get_sync(pdata->dev);
> > + edid = pdata->edid = drm_get_edid(connector, &pdata->aux.ddc);
> > + pm_runtime_put(pdata->dev);
>
> Is there any specific reason to use the indirect access method, compared
> to the direct method that translates access to an I2C ancillary address
> to an I2C-over-AUX transaction (see page 20 of SLLSEH2B) ? The direct
> method seems it would be more efficient.
>
No I don't think it matters. I was just using the existing support code
that Sean wrote instead of digging into the details. Maybe Sean ran into
something earlier and abandoned that approach?
Hi,
On Mon, Mar 22, 2021 at 8:17 PM Stephen Boyd <[email protected]> wrote:
>
> Quoting Laurent Pinchart (2021-03-17 17:20:43)
> > Hi Stephen,
> >
> > Reviving a bit of an old thread, for a question.
> >
> > On Mon, Nov 02, 2020 at 10:11:43AM -0800, Stephen Boyd wrote:
> > > @@ -265,6 +267,23 @@ connector_to_ti_sn_bridge(struct drm_connector *connector)
> > > static int ti_sn_bridge_connector_get_modes(struct drm_connector *connector)
> > > {
> > > struct ti_sn_bridge *pdata = connector_to_ti_sn_bridge(connector);
> > > + struct edid *edid = pdata->edid;
> > > + int num, ret;
> > > +
> > > + if (!edid) {
> > > + pm_runtime_get_sync(pdata->dev);
> > > + edid = pdata->edid = drm_get_edid(connector, &pdata->aux.ddc);
> > > + pm_runtime_put(pdata->dev);
> >
> > Is there any specific reason to use the indirect access method, compared
> > to the direct method that translates access to an I2C ancillary address
> > to an I2C-over-AUX transaction (see page 20 of SLLSEH2B) ? The direct
> > method seems it would be more efficient.
> >
>
> No I don't think it matters. I was just using the existing support code
> that Sean wrote instead of digging into the details. Maybe Sean ran into
> something earlier and abandoned that approach?
From reading the docs, it sounds as if there _could_ be a reason to
use the indirect method. Specifically if the i2c host that the bridge
is on doesn't support clock stretching then the direct method wouldn't
work according to the docs. Is that something that we'd have to
reasonably worry about?
-Doug
Hi Doug,
On Tue, Mar 23, 2021 at 12:07:27PM -0700, Doug Anderson wrote:
> On Mon, Mar 22, 2021 at 8:17 PM Stephen Boyd <[email protected]> wrote:
> >
> > Quoting Laurent Pinchart (2021-03-17 17:20:43)
> > > Hi Stephen,
> > >
> > > Reviving a bit of an old thread, for a question.
> > >
> > > On Mon, Nov 02, 2020 at 10:11:43AM -0800, Stephen Boyd wrote:
> > > > @@ -265,6 +267,23 @@ connector_to_ti_sn_bridge(struct drm_connector *connector)
> > > > static int ti_sn_bridge_connector_get_modes(struct drm_connector *connector)
> > > > {
> > > > struct ti_sn_bridge *pdata = connector_to_ti_sn_bridge(connector);
> > > > + struct edid *edid = pdata->edid;
> > > > + int num, ret;
> > > > +
> > > > + if (!edid) {
> > > > + pm_runtime_get_sync(pdata->dev);
> > > > + edid = pdata->edid = drm_get_edid(connector, &pdata->aux.ddc);
> > > > + pm_runtime_put(pdata->dev);
> > >
> > > Is there any specific reason to use the indirect access method, compared
> > > to the direct method that translates access to an I2C ancillary address
> > > to an I2C-over-AUX transaction (see page 20 of SLLSEH2B) ? The direct
> > > method seems it would be more efficient.
> >
> > No I don't think it matters. I was just using the existing support code
> > that Sean wrote instead of digging into the details. Maybe Sean ran into
> > something earlier and abandoned that approach?
>
> From reading the docs, it sounds as if there _could_ be a reason to
> use the indirect method. Specifically if the i2c host that the bridge
> is on doesn't support clock stretching then the direct method wouldn't
> work according to the docs. Is that something that we'd have to
> reasonably worry about?
I'm not sure. I'm going through BSP code that uses the direct method,
and I was wondering if it was just an implementation detail. Once I get
the display working on this board, I'll try to find time to compare the
two methods, to see if there's a significatant performance improvement
from the direct method. If there isn't, I won't bother.
--
Regards,
Laurent Pinchart
Hi,
On Tue, Mar 23, 2021 at 12:53 PM Laurent Pinchart
<[email protected]> wrote:
>
> Hi Doug,
>
> On Tue, Mar 23, 2021 at 12:07:27PM -0700, Doug Anderson wrote:
> > On Mon, Mar 22, 2021 at 8:17 PM Stephen Boyd <[email protected]> wrote:
> > >
> > > Quoting Laurent Pinchart (2021-03-17 17:20:43)
> > > > Hi Stephen,
> > > >
> > > > Reviving a bit of an old thread, for a question.
> > > >
> > > > On Mon, Nov 02, 2020 at 10:11:43AM -0800, Stephen Boyd wrote:
> > > > > @@ -265,6 +267,23 @@ connector_to_ti_sn_bridge(struct drm_connector *connector)
> > > > > static int ti_sn_bridge_connector_get_modes(struct drm_connector *connector)
> > > > > {
> > > > > struct ti_sn_bridge *pdata = connector_to_ti_sn_bridge(connector);
> > > > > + struct edid *edid = pdata->edid;
> > > > > + int num, ret;
> > > > > +
> > > > > + if (!edid) {
> > > > > + pm_runtime_get_sync(pdata->dev);
> > > > > + edid = pdata->edid = drm_get_edid(connector, &pdata->aux.ddc);
> > > > > + pm_runtime_put(pdata->dev);
> > > >
> > > > Is there any specific reason to use the indirect access method, compared
> > > > to the direct method that translates access to an I2C ancillary address
> > > > to an I2C-over-AUX transaction (see page 20 of SLLSEH2B) ? The direct
> > > > method seems it would be more efficient.
> > >
> > > No I don't think it matters. I was just using the existing support code
> > > that Sean wrote instead of digging into the details. Maybe Sean ran into
> > > something earlier and abandoned that approach?
> >
> > From reading the docs, it sounds as if there _could_ be a reason to
> > use the indirect method. Specifically if the i2c host that the bridge
> > is on doesn't support clock stretching then the direct method wouldn't
> > work according to the docs. Is that something that we'd have to
> > reasonably worry about?
>
> I'm not sure. I'm going through BSP code that uses the direct method,
> and I was wondering if it was just an implementation detail. Once I get
> the display working on this board, I'll try to find time to compare the
> two methods, to see if there's a significatant performance improvement
> from the direct method. If there isn't, I won't bother.
To follow-up here:
We'd actually been using the "direct" method in the BIOS (coreboot)
and just found a problem. We're now switching coreboot to the
"indirect" mode. Specifically we found that, at least on one panel,
the last byte of the extension block (which should have been a CRC)
was coming back as 0 when using the "direct" mode. See:
https://review.coreboot.org/c/coreboot/+/52959
In addition I was thinking about how to use "direct" mode (ignoring
the above problem) and realized that handling the power sequencing at
the right time would be hard. Maybe not a problem for you since your
bridge is always powered, but I wouldn't know how to model this in
general. Specifically if you want to talk over the i2c bus to the
panel you've got to power the bridge but I don't think the bridge gets
called in the normal code paths.
-Doug