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 | 108 ++++++++++++++++++-------- 1 file changed, 75 insertions(+), 33 deletions(-)
Cc: Douglas Anderson dianders@chromium.org Cc: Laurent Pinchart Laurent.pinchart@ideasonboard.com Cc: Jonas Karlman jonas@kwiboo.se Cc: Jernej Skrabec jernej.skrabec@siol.net Cc: Sean Paul seanpaul@chromium.org
base-commit: 3650b228f83adda7e5ee532e2b90429c03f7b9ec
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 v2: - Combined AUX_CMD register write
Cc: Douglas Anderson dianders@chromium.org Cc: Laurent Pinchart Laurent.pinchart@ideasonboard.com Cc: Jonas Karlman jonas@kwiboo.se Cc: Jernej Skrabec jernej.skrabec@siol.net Cc: Sean Paul seanpaul@chromium.org Signed-off-by: Stephen Boyd swboyd@chromium.org --- drivers/gpu/drm/bridge/ti-sn65dsi86.c | 52 ++++++++++++--------------- 1 file changed, 23 insertions(+), 29 deletions(-)
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c index ecdf9b01340f..a1ebfa95088c 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 reg_buf[SN_AUX_CMD_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) { @@ -853,25 +858,20 @@ static ssize_t ti_sn_aux_transfer(struct drm_dp_aux *aux, case DP_AUX_I2C_WRITE: case DP_AUX_NATIVE_READ: case DP_AUX_I2C_READ: - regmap_write(pdata->regmap, SN_AUX_CMD_REG, request_val); break; default: 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(reg_buf) < sizeof(__be32)); + put_unaligned_be32((msg->address & SN_AUX_ADDR_MASK) << 8 | len, + reg_buf); + reg_buf[SN_AUX_CMD_REG - SN_AUX_ADDR_19_16_REG] = request_val; + regmap_bulk_write(pdata->regmap, SN_AUX_ADDR_19_16_REG, reg_buf, + ARRAY_SIZE(reg_buf));
- 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)
Hi,
On Thu, Oct 29, 2020 at 6:17 PM Stephen Boyd swboyd@chromium.org wrote:
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 v2:
- Combined AUX_CMD register write
The change from v1 to v2 makes me slightly nervous, though I guess it's fine. Specifically, all the examples in the datasheet show programming the CMD before the ADDR and LEN. This change will make it programmed after. Since there's a separate START bit I guess it's OK, though. Nothing in the datasheet explicitly says that the order in the examples is the only order that will work...
Reviewed-by: Douglas Anderson dianders@chromium.org
Quoting Doug Anderson (2020-11-02 08:18:47)
Hi,
On Thu, Oct 29, 2020 at 6:17 PM Stephen Boyd swboyd@chromium.org wrote:
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 v2:
- Combined AUX_CMD register write
The change from v1 to v2 makes me slightly nervous, though I guess it's fine. Specifically, all the examples in the datasheet show programming the CMD before the ADDR and LEN. This change will make it programmed after. Since there's a separate START bit I guess it's OK, though. Nothing in the datasheet explicitly says that the order in the examples is the only order that will work...
Hmmm now that you mention it the SEND bit is explicitly being cleared in the programming sequence by being there at the start. If I want to combine that with the adjacent register writes then I should make sure that the SEND bit is cleared at the beginning. Otherwise the hardware may be in the middle of a transaction if the previous transaction is still running, i.e. a timeout where the SEND bit never cleared.
I think we should go back to the previous patch I had here. Combining this register write is wrong. If anything, we should clear the SEND bit on a timeout and make sure during probe that this bit is clear and then drop the programming of this register from this function entirely. That would reduce the sequence by one register, but is more complicated vs. just making sure it has the clear bit here to begin with.
Reviewed-by: Douglas Anderson dianders@chromium.org
Thanks, but I'll send another round picking up acks and such and your previous review tag on the v1 of this patch.
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 dianders@chromium.org Cc: Laurent Pinchart Laurent.pinchart@ideasonboard.com Cc: Jonas Karlman jonas@kwiboo.se Cc: Jernej Skrabec jernej.skrabec@siol.net Cc: Sean Paul seanpaul@chromium.org Signed-off-by: Stephen Boyd swboyd@chromium.org --- 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 a1ebfa95088c..c77f46a21aae 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;
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 dianders@chromium.org Cc: Laurent Pinchart Laurent.pinchart@ideasonboard.com Cc: Jonas Karlman jonas@kwiboo.se Cc: Jernej Skrabec jernej.skrabec@siol.net Cc: Sean Paul seanpaul@chromium.org Signed-off-by: Stephen Boyd swboyd@chromium.org --- 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 c77f46a21aae..f86934fd6cc8 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);
Hi Stephen,
Thank you for the patch.
On Thu, Oct 29, 2020 at 06:17:37PM -0700, 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 dianders@chromium.org Cc: Laurent Pinchart Laurent.pinchart@ideasonboard.com Cc: Jonas Karlman jonas@kwiboo.se Cc: Jernej Skrabec jernej.skrabec@siol.net Cc: Sean Paul seanpaul@chromium.org Signed-off-by: Stephen Boyd swboyd@chromium.org
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 c77f46a21aae..f86934fd6cc8 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);- }
Do we need to cache the EDID ? It seems like something that should be done by the DRM core (well, caching modes in that case), not by individual bridge drivers.
Apart from that,
Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com
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);
Hi,
On Sun, Nov 1, 2020 at 11:21 AM Laurent Pinchart laurent.pinchart@ideasonboard.com wrote:
Hi Stephen,
Thank you for the patch.
On Thu, Oct 29, 2020 at 06:17:37PM -0700, 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 dianders@chromium.org Cc: Laurent Pinchart Laurent.pinchart@ideasonboard.com Cc: Jonas Karlman jonas@kwiboo.se Cc: Jernej Skrabec jernej.skrabec@siol.net Cc: Sean Paul seanpaul@chromium.org Signed-off-by: Stephen Boyd swboyd@chromium.org
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 c77f46a21aae..f86934fd6cc8 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);}Do we need to cache the EDID ? It seems like something that should be done by the DRM core (well, caching modes in that case), not by individual bridge drivers.
I can take the blame for the fact that it does caching, since I requested it in early reviews. In general boot speed is pretty important to me and each read of the EDID take 20 ms. There are definitely several calls to get the EDID during a normal bootup. Stephen did a little more digging into exactly what was causing all these calls and can chime in, but in general until we can eliminate the extra calls it seems like it'd be nice to keep the caching? This bridge chip is intended for use for eDP for internal panels, so there should be no downside to caching. If we can later optimize the DRM core, we can fix this and a pre-existing driver that does the same type of caching (analogix-anx6345.c) at the same time?
-Doug
Quoting Doug Anderson (2020-11-02 08:06:14)
On Sun, Nov 1, 2020 at 11:21 AM Laurent Pinchart laurent.pinchart@ideasonboard.com wrote:
On Thu, Oct 29, 2020 at 06:17:37PM -0700, 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);}Do we need to cache the EDID ? It seems like something that should be done by the DRM core (well, caching modes in that case), not by individual bridge drivers.
I can take the blame for the fact that it does caching, since I requested it in early reviews. In general boot speed is pretty important to me and each read of the EDID take 20 ms. There are definitely several calls to get the EDID during a normal bootup. Stephen did a little more digging into exactly what was causing all these calls and can chime in,
In ChromeOS we get modes a couple times and then whenever we connect or disconnect a DP cable for external display we also get modes. It seems that we also run modetest at boot but I'm not sure why we do that. I think it is to gather diagnostic data for all the EDIDs on the device at boot so we know what all is connected.
but in general until we can eliminate the extra calls it seems like it'd be nice to keep the caching? This bridge chip is intended for use for eDP for internal panels, so there should be no downside to caching. If we can later optimize the DRM core, we can fix this and a pre-existing driver that does the same type of caching (analogix-anx6345.c) at the same time?
I'd like to add the caching somewhere in the core (maybe the bridge connector code?) but I don't know what the logic should be. Is it eDP and if not hpd notify then cache all the time and if it is eDP and hpd notify then cache once hpd notify says detected and drop cache when no longer detected?
if (eDP) { if (!hpd) cache(); else if (hpd_detected()) { cache(); else if (!hpd_detected()) { drop_cache(); } }
I thought that EDID could change and HPD can be pulsed to notify that it should be read again.
Hi Stephen,
On Mon, Nov 02, 2020 at 09:38:12AM -0800, Stephen Boyd wrote:
Quoting Doug Anderson (2020-11-02 08:06:14)
On Sun, Nov 1, 2020 at 11:21 AM Laurent Pinchart wrote:
On Thu, Oct 29, 2020 at 06:17:37PM -0700, 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);}Do we need to cache the EDID ? It seems like something that should be done by the DRM core (well, caching modes in that case), not by individual bridge drivers.
I can take the blame for the fact that it does caching, since I requested it in early reviews. In general boot speed is pretty important to me and each read of the EDID take 20 ms. There are definitely several calls to get the EDID during a normal bootup. Stephen did a little more digging into exactly what was causing all these calls and can chime in,
In ChromeOS we get modes a couple times and then whenever we connect or disconnect a DP cable for external display we also get modes. It seems that we also run modetest at boot but I'm not sure why we do that. I think it is to gather diagnostic data for all the EDIDs on the device at boot so we know what all is connected.
but in general until we can eliminate the extra calls it seems like it'd be nice to keep the caching? This bridge chip is intended for use for eDP for internal panels, so there should be no downside to caching. If we can later optimize the DRM core, we can fix this and a pre-existing driver that does the same type of caching (analogix-anx6345.c) at the same time?
I'd like to add the caching somewhere in the core (maybe the bridge connector code?) but I don't know what the logic should be. Is it eDP and if not hpd notify then cache all the time and if it is eDP and hpd notify then cache once hpd notify says detected and drop cache when no longer detected?
if (eDP) { if (!hpd) cache(); else if (hpd_detected()) { cache(); else if (!hpd_detected()) { drop_cache(); } }
I thought that EDID could change and HPD can be pulsed to notify that it should be read again.
I think we should expose a flag tells the panel is fixed instead of making it a special case of eDP, as other panel types could benefit from the same mechanism. Otherwise, yes, I think it's really about caching the EDID the first time we read it, and then reusing it. The question is who should convert EDID to modes. At the moment bridge drivers do so, and we're migrating to drm_bridge_connector for most cases. That would be a candidate location to cache EDID. The DRM core would be another one, but in that case we may need to also cache the modes.
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: - Handle WRITE_STATUS_UPDATE properly
Cc: Douglas Anderson dianders@chromium.org Cc: Laurent Pinchart Laurent.pinchart@ideasonboard.com Cc: Jonas Karlman jonas@kwiboo.se Cc: Jernej Skrabec jernej.skrabec@siol.net Cc: Sean Paul seanpaul@chromium.org Signed-off-by: Stephen Boyd swboyd@chromium.org --- drivers/gpu/drm/bridge/ti-sn65dsi86.c | 36 ++++++++++++++++++++++++--- 1 file changed, 32 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c index f86934fd6cc8..984ea41deca8 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c @@ -873,10 +873,16 @@ static ssize_t ti_sn_aux_transfer(struct drm_dp_aux *aux, return -EINVAL;
switch (request) { + case DP_AUX_I2C_WRITE_STATUS_UPDATE: + /* WRITE_STATUS_UPDATE only matters for request_val */ + request &= ~DP_AUX_I2C_WRITE_STATUS_UPDATE; + fallthrough; case DP_AUX_NATIVE_WRITE: case DP_AUX_I2C_WRITE: case DP_AUX_NATIVE_READ: case DP_AUX_I2C_READ: + /* Assume it's good */ + msg->reply = 0; break; default: return -EINVAL; @@ -909,10 +915,32 @@ 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)
Hi,
On Thu, Oct 29, 2020 at 6:17 PM Stephen Boyd swboyd@chromium.org wrote:
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:
- Handle WRITE_STATUS_UPDATE properly
Cc: Douglas Anderson dianders@chromium.org Cc: Laurent Pinchart Laurent.pinchart@ideasonboard.com Cc: Jonas Karlman jonas@kwiboo.se Cc: Jernej Skrabec jernej.skrabec@siol.net Cc: Sean Paul seanpaul@chromium.org Signed-off-by: Stephen Boyd swboyd@chromium.org
drivers/gpu/drm/bridge/ti-sn65dsi86.c | 36 ++++++++++++++++++++++++--- 1 file changed, 32 insertions(+), 4 deletions(-)
This looks right to me, now. Hopefully if/when someone ends up with hardware that exercises these codepaths they'll at least be in a better state and maybe they will all just work! :-)
Reviewed-by: Douglas Anderson dianders@chromium.org
Hi Stephen.
On Thu, Oct 29, 2020 at 06:17:34PM -0700, 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
Series looks good. You can add my a-b on the full series. Acked-by: Sam Ravnborg sam@ravnborg.org
I can apply after Douglas have had a look at the patches he did not r-b yet.
Any chance we can convince you to prepare this bridge driver for use in a chained bridge setup where the connector is created by the display driver and uses drm_bridge_funcs?
First step wuld be to introduce the use of a panel_bridge. Then add get_edid to drm_bridge_funcs and maybe more helpers.
Then natural final step would be to move connector creation to the display driver - see how other uses drm_bridge_connector_init() to do so - it is relatively simple.
Should be doable - and reach out if you need some help.
Sam
drivers/gpu/drm/bridge/ti-sn65dsi86.c | 108 ++++++++++++++++++-------- 1 file changed, 75 insertions(+), 33 deletions(-)
Cc: Douglas Anderson dianders@chromium.org Cc: Laurent Pinchart Laurent.pinchart@ideasonboard.com Cc: Jonas Karlman jonas@kwiboo.se Cc: Jernej Skrabec jernej.skrabec@siol.net Cc: Sean Paul seanpaul@chromium.org
base-commit: 3650b228f83adda7e5ee532e2b90429c03f7b9ec
Sent by a computer, using git, on the internet
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Hi,
On Sun, Nov 1, 2020 at 9:37 AM Sam Ravnborg sam@ravnborg.org wrote:
Hi Stephen.
On Thu, Oct 29, 2020 at 06:17:34PM -0700, 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
Series looks good. You can add my a-b on the full series. Acked-by: Sam Ravnborg sam@ravnborg.org
I can apply after Douglas have had a look at the patches he did not r-b yet.
They look fine to me now assuming that Stepehn has tested patch #1 enough that we're confident that the slight change in ordering isn't going to mess anything up. Laurent also had comments about caching the EDID on patch #3. If he feels strongly about getting rid of that, it'll need another spin and we'll just have to suck up the small boot time penalty until we can find a solution in the core.
Any chance we can convince you to prepare this bridge driver for use in a chained bridge setup where the connector is created by the display driver and uses drm_bridge_funcs?
First step wuld be to introduce the use of a panel_bridge. Then add get_edid to drm_bridge_funcs and maybe more helpers.
Then natural final step would be to move connector creation to the display driver - see how other uses drm_bridge_connector_init() to do so
- it is relatively simple.
Should be doable - and reach out if you need some help.
At some point I think Vinod tried to prepare a patch for this and I tried it, but it didn't just work. I spent an hour or so poking at it and I couldn't quite figure out why and I couldn't find enough other examples to compare against to see what was wrong... That was a few months ago, though. Maybe things are in a better shape now?
-Doug
Quoting Doug Anderson (2020-11-02 08:37:21)
Hi,
On Sun, Nov 1, 2020 at 9:37 AM Sam Ravnborg sam@ravnborg.org wrote:
Hi Stephen.
On Thu, Oct 29, 2020 at 06:17:34PM -0700, 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
Series looks good. You can add my a-b on the full series. Acked-by: Sam Ravnborg sam@ravnborg.org
I can apply after Douglas have had a look at the patches he did not r-b yet.
They look fine to me now assuming that Stepehn has tested patch #1 enough that we're confident that the slight change in ordering isn't going to mess anything up.
I did test it but the test isn't thorough enough to cover the timeout case. I'll resend with v1 of this patch and pick up acks and include Sam on To line.
Hi,
Thanks Doug for adding me
On 02-11-20, 08:37, Doug Anderson wrote:
On Thu, Oct 29, 2020 at 06:17:34PM -0700, Stephen Boyd wrote:
Any chance we can convince you to prepare this bridge driver for use in a chained bridge setup where the connector is created by the display driver and uses drm_bridge_funcs?
First step wuld be to introduce the use of a panel_bridge. Then add get_edid to drm_bridge_funcs and maybe more helpers.
Then natural final step would be to move connector creation to the display driver - see how other uses drm_bridge_connector_init() to do so
- it is relatively simple.
Should be doable - and reach out if you need some help.
Yes it is and doable and you find this at [1], would need a rebase though.
At some point I think Vinod tried to prepare a patch for this and I tried it, but it didn't just work. I spent an hour or so poking at it and I couldn't quite figure out why and I couldn't find enough other examples to compare against to see what was wrong... That was a few months ago, though. Maybe things are in a better shape now?
It worked fine for me on Rb3 and db410c where we had HDMI connector. I don't have a panel device to test and Bjorn tried to help out with a bit of testing. This didn't work on the laptop, that is why I haven't posted it yet.
This has conversion of msm driver and bridge drivers lt9611, adv7511 and ti-sn65dsi86.
[1]: https://git.linaro.org/people/vinod.koul/kernel.git/log/?h=wip/msm_bridges_n...
Thanks
Quoting Sam Ravnborg (2020-11-01 09:37:41)
Hi Stephen.
On Thu, Oct 29, 2020 at 06:17:34PM -0700, 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
Series looks good. You can add my a-b on the full series. Acked-by: Sam Ravnborg sam@ravnborg.org
I can apply after Douglas have had a look at the patches he did not r-b yet.
Any chance we can convince you to prepare this bridge driver for use in a chained bridge setup where the connector is created by the display driver and uses drm_bridge_funcs?
First step wuld be to introduce the use of a panel_bridge. Then add get_edid to drm_bridge_funcs and maybe more helpers.
Then natural final step would be to move connector creation to the display driver - see how other uses drm_bridge_connector_init() to do so
- it is relatively simple.
Should be doable - and reach out if you need some help.
I started to look at this and got stuck at ti_sn_bridge_get_bpp(). Where can I get the details of the bpc for the downstream bridge or panel? Is there some function that can tell this bridge what the bpc is for the attached connector? I see that td_mode_valid() in drivers/gpu/drm/bridge/tc358775.c stores away the bpc from the incoming drm_display_info pointer but I'm not sure that is correct because can't that be called for various and not necessarily the one we're using?
Quoting Stephen Boyd (2020-11-02 17:15:24)
Quoting Sam Ravnborg (2020-11-01 09:37:41)
Hi Stephen.
On Thu, Oct 29, 2020 at 06:17:34PM -0700, 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
Series looks good. You can add my a-b on the full series. Acked-by: Sam Ravnborg sam@ravnborg.org
I can apply after Douglas have had a look at the patches he did not r-b yet.
Any chance we can convince you to prepare this bridge driver for use in a chained bridge setup where the connector is created by the display driver and uses drm_bridge_funcs?
First step wuld be to introduce the use of a panel_bridge. Then add get_edid to drm_bridge_funcs and maybe more helpers.
Then natural final step would be to move connector creation to the display driver - see how other uses drm_bridge_connector_init() to do so
- it is relatively simple.
Should be doable - and reach out if you need some help.
I started to look at this and got stuck at ti_sn_bridge_get_bpp(). Where can I get the details of the bpc for the downstream bridge or panel? Is there some function that can tell this bridge what the bpc is for the attached connector? I see that td_mode_valid() in drivers/gpu/drm/bridge/tc358775.c stores away the bpc from the incoming drm_display_info pointer but I'm not sure that is correct because can't that be called for various and not necessarily the one we're using?
Looking closer I see that tc358775 doesn't store away the incoming value, it derives a bpc from what comes in. I guess it isn't really a good example for this problem.
I tried to hack around this and convert this driver to use bridge funcs with Vinod's msm patch underneath but now the encoder is invalid. I wonder if something is wrong with how the msm display driver drives the bridge and connector? That TODO in ti_sn_bridge_attach() makes me think something else needs fixing. I'll send another patch series tomorrow on top of this one that kicks off the conversion to bridge connector so we can discuss there.
[ 4.577814] [drm:dpu_core_perf_crtc_update:397] [dpu error]crtc-49: failed to update bus bw vote [ 4.603697] panel-simple panel: error waiting for hpd GPIO: -6 [ 4.613106] dsi_calc_pclk: forcing mdss_dsi lanes to 1 [ 4.619070] dsi_link_clk_set_rate_6g: Failed to set rate pixel clk, -22 [ 4.625883] msm_dsi_host_power_on: failed to enable link clocks. ret=-22 [ 4.632784] dsi_mgr_bridge_pre_enable: power on host 0 failed, -22 [ 4.646235] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000370 [ 4.655279] Mem abort info: [ 4.658249] ESR = 0x96000006 [ 4.661456] EC = 0x25: DABT (current EL), IL = 32 bits [ 4.667047] SET = 0, FnV = 0 [ 4.670195] EA = 0, S1PTW = 0 [ 4.673430] Data abort info: [ 4.676401] ISV = 0, ISS = 0x00000006 [ 4.680347] CM = 0, WnR = 0 [ 4.683413] user pgtable: 4k pages, 39-bit VAs, pgdp=0000000104f18000 [ 4.690033] [0000000000000370] pgd=0000000104f21003, p4d=0000000104f21003, pud=0000000104f21003, pmd=0000000000000000 [ 4.700941] Internal error: Oops: 96000006 [#1] PREEMPT SMP [ 4.706672] Modules linked in: cdc_ether usbnet r8152 joydev mii [ 4.712866] CPU: 6 PID: 376 Comm: frecon Not tainted 5.10.0-rc2+ #5 [ 4.719302] Hardware name: Google Lazor (rev1+) with LTE (DT) [ 4.725201] pstate: 60400009 (nZCv daif +PAN -UAO -TCO BTYPE=--) [ 4.731390] pc : ti_sn_bridge_enable+0xa4/0x7ec [ 4.736047] lr : ti_sn_bridge_enable+0x54/0x7ec [ 4.740704] sp : ffffffc0109db960 [ 4.744119] x29: ffffffc0109db990 x28: ffffffd216acb000 [ 4.749586] x27: ffffffd216854000 x26: ffffffd216acb000 [ 4.755046] x25: ffffffd2164e89d0 x24: 0000000000000000 [ 4.760508] x23: 0000000000000000 x22: ffffff8281de5080 [ 4.765970] x21: ffffff8281de56f0 x20: ffffff8281de5670 [ 4.771431] x19: ffffff8281de5090 x18: 0000000000000484 [ 4.776893] x17: 0000000000000000 x16: 0000000000000001 [ 4.782355] x15: 0000000000000010 x14: 0000000000000000 [ 4.787816] x13: 00000000000013a8 x12: ffffff828208f950 [ 4.793277] x11: ffffff82818e1d80 x10: 0000000000000000 [ 4.798738] x9 : 0000000000000002 x8 : 0000000000000002 [ 4.804200] x7 : ffffffc0109db978 x6 : 0000000000000001 [ 4.809660] x5 : 0000000000000001 x4 : ffffffd2166b6388 [ 4.815122] x3 : 0000000000000001 x2 : ffffffd216680dab [ 4.820584] x1 : 0000000000000010 x0 : ffffff8280ceac00 [ 4.826046] Call trace: [ 4.828568] ti_sn_bridge_enable+0xa4/0x7ec [ 4.832886] drm_atomic_bridge_chain_enable+0x7c/0xa4 [ 4.838095] drm_atomic_helper_commit_modeset_enables+0x19c/0x23c [ 4.844360] msm_atomic_commit_tail+0x308/0x6e8 [ 4.849017] commit_tail+0xa8/0x140 [ 4.852608] drm_atomic_helper_commit+0xf8/0x100 [ 4.857354] drm_atomic_commit+0x50/0x5c [ 4.861385] drm_atomic_helper_set_config+0xcc/0xd4 [ 4.866398] drm_mode_setcrtc+0x280/0x5c0 [ 4.870519] drm_ioctl_kernel+0xa0/0x118 [ 4.874549] drm_ioctl+0x240/0x3dc
Hi Stephen,
On Mon, Nov 02, 2020 at 05:15:24PM -0800, Stephen Boyd wrote:
Quoting Sam Ravnborg (2020-11-01 09:37:41)
Hi Stephen.
On Thu, Oct 29, 2020 at 06:17:34PM -0700, 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
Series looks good. You can add my a-b on the full series. Acked-by: Sam Ravnborg sam@ravnborg.org
I can apply after Douglas have had a look at the patches he did not r-b yet.
Any chance we can convince you to prepare this bridge driver for use in a chained bridge setup where the connector is created by the display driver and uses drm_bridge_funcs?
First step wuld be to introduce the use of a panel_bridge. Then add get_edid to drm_bridge_funcs and maybe more helpers.
Then natural final step would be to move connector creation to the display driver - see how other uses drm_bridge_connector_init() to do so
- it is relatively simple.
Should be doable - and reach out if you need some help.
I started to look at this and got stuck at ti_sn_bridge_get_bpp(). Where can I get the details of the bpc for the downstream bridge or panel? Is there some function that can tell this bridge what the bpc is for the attached connector?
I've posted a patch series to convert to DRM_BRIDGE_ATTACH_NO_CONNECTOR yesterday (and have CC'ed you), but I've overlooked this particular problem :-S
You can't get the connector in the .enable() operation, but you can get it in .atomic_enable(), with drm_atomic_get_new_connector_for_encoder(). This being said, it's probably not the right option.
What matters here isn't the bpc for the connector, but the format expected by the next bridge in the chain. drm_bridge_funcs has two operations, .atomic_get_output_bus_fmts() and .atomic_get_input_bus_fmts(), to negotiate that format along a chain of bridges. The panel bridge driver (drivers/gpu/drm/bridge/panel.c) doesn't implement those operations, and neither does display-connector.c, so that may be what we should start with.
I see that td_mode_valid() in drivers/gpu/drm/bridge/tc358775.c stores away the bpc from the incoming drm_display_info pointer but I'm not sure that is correct because can't that be called for various and not necessarily the one we're using?
You're right, .mode_valid() shouldn't do that.
Quoting Laurent Pinchart (2021-03-22 17:00:23)
Hi Stephen,
On Mon, Nov 02, 2020 at 05:15:24PM -0800, Stephen Boyd wrote:
Quoting Sam Ravnborg (2020-11-01 09:37:41)
Hi Stephen.
On Thu, Oct 29, 2020 at 06:17:34PM -0700, 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
Series looks good. You can add my a-b on the full series. Acked-by: Sam Ravnborg sam@ravnborg.org
I can apply after Douglas have had a look at the patches he did not r-b yet.
Any chance we can convince you to prepare this bridge driver for use in a chained bridge setup where the connector is created by the display driver and uses drm_bridge_funcs?
First step wuld be to introduce the use of a panel_bridge. Then add get_edid to drm_bridge_funcs and maybe more helpers.
Then natural final step would be to move connector creation to the display driver - see how other uses drm_bridge_connector_init() to do so
- it is relatively simple.
Should be doable - and reach out if you need some help.
I started to look at this and got stuck at ti_sn_bridge_get_bpp(). Where can I get the details of the bpc for the downstream bridge or panel? Is there some function that can tell this bridge what the bpc is for the attached connector?
I've posted a patch series to convert to DRM_BRIDGE_ATTACH_NO_CONNECTOR yesterday (and have CC'ed you), but I've overlooked this particular problem :-S
!
You can't get the connector in the .enable() operation, but you can get it in .atomic_enable(), with drm_atomic_get_new_connector_for_encoder(). This being said, it's probably not the right option.
What matters here isn't the bpc for the connector, but the format expected by the next bridge in the chain. drm_bridge_funcs has two operations, .atomic_get_output_bus_fmts() and .atomic_get_input_bus_fmts(), to negotiate that format along a chain of bridges. The panel bridge driver (drivers/gpu/drm/bridge/panel.c) doesn't implement those operations, and neither does display-connector.c, so that may be what we should start with.
Ok, makes sense. I'd gladly test things out if you come up with some solution here.
dri-devel@lists.freedesktop.org
-
Doug Anderson -
Laurent Pinchart -
Sam Ravnborg -
Stephen Boyd -
Vinod Koul