diff mbox series

[v2,3/4] drm/bridge: ti-sn65dsi86: Read EDID blob over DDC

Message ID 20201030011738.2028313-4-swboyd@chromium.org
State New, archived
Headers show
Series drm/bridge: ti-sn65dsi86: Support EDID reading | expand

Commit Message

Stephen Boyd Oct. 30, 2020, 1:17 a.m. UTC
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(+)

Comments

Laurent Pinchart Nov. 1, 2020, 7:20 p.m. UTC | #1
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);
Doug Anderson Nov. 2, 2020, 4:06 p.m. UTC | #2
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
Stephen Boyd Nov. 2, 2020, 5:38 p.m. UTC | #3
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.
Laurent Pinchart Nov. 2, 2020, 9:55 p.m. UTC | #4
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.
diff mbox series

Patch

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);