Re: [PATCH v3 3/4] drm/bridge: ti-sn65dsi86: Read EDID blob over DDC
Hi,
On Tue, Mar 23, 2021 at 12:53 PM Laurent Pinchart
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 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,
> > > > > >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
Re: [PATCH v3 3/4] drm/bridge: ti-sn65dsi86: Read EDID blob over DDC
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 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,
> > > > >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
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v3 3/4] drm/bridge: ti-sn65dsi86: Read EDID blob over DDC
Hi,
On Mon, Mar 22, 2021 at 8:17 PM Stephen Boyd 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,
> > > >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
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v3 3/4] drm/bridge: ti-sn65dsi86: Read EDID blob over DDC
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, >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?
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v3 3/4] drm/bridge: ti-sn65dsi86: Read EDID blob over DDC
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
> Reviewed-by: Laurent Pinchart
> Cc: Jonas Karlman
> Cc: Jernej Skrabec
> Cc: Sean Paul
> Acked-by: Sam Ravnborg
> Signed-off-by: Stephen Boyd
> ---
> 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_connectorconnector;
> 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, >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
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v3 3/4] drm/bridge: ti-sn65dsi86: Read EDID blob over DDC
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
Reviewed-by: Laurent Pinchart
Cc: Jonas Karlman
Cc: Jernej Skrabec
Cc: Sean Paul
Acked-by: Sam Ravnborg
Signed-off-by: Stephen Boyd
---
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_connectorconnector;
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, >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
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
