diff mbox series

[RFC,08/11] drm/bridge: ti-sn65dsi86: Implement bridge connector operations

Message ID 20210322030128.2283-9-laurent.pinchart+renesas@ideasonboard.com (mailing list archive)
State New
Delegated to: Kieran Bingham
Headers show
Series drm/bridge: ti-sn65dsi86: Support DisplayPort mode | expand

Commit Message

Laurent Pinchart March 22, 2021, 3:01 a.m. UTC
Implement the bridge connector-related .get_edid() operation, and report
the related bridge capabilities and type. The .get_edid() operation is
implemented with the same backend as the EDID retrieval from the
connector .get_modes() operation.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 drivers/gpu/drm/bridge/ti-sn65dsi86.c | 30 ++++++++++++++++++++++-----
 1 file changed, 25 insertions(+), 5 deletions(-)

Comments

Stephen Boyd March 23, 2021, 7:15 a.m. UTC | #1
Quoting Laurent Pinchart (2021-03-21 20:01:25)
> Implement the bridge connector-related .get_edid() operation, and report
> the related bridge capabilities and type. The .get_edid() operation is
> implemented with the same backend as the EDID retrieval from the
> connector .get_modes() operation.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> ---

Reviewed-by: Stephen Boyd <swboyd@chromium.org>
Doug Anderson March 24, 2021, 10:46 p.m. UTC | #2
Hi,

On Sun, Mar 21, 2021 at 8:02 PM Laurent Pinchart
<laurent.pinchart+renesas@ideasonboard.com> wrote:
>
> Implement the bridge connector-related .get_edid() operation, and report
> the related bridge capabilities and type. The .get_edid() operation is
> implemented with the same backend as the EDID retrieval from the
> connector .get_modes() operation.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> ---
>  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 30 ++++++++++++++++++++++-----
>  1 file changed, 25 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index dc300fab4319..6f6e075544e8 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -261,6 +261,18 @@ static void ti_sn_debugfs_remove(struct ti_sn_bridge *pdata)
>         pdata->debugfs = NULL;
>  }
>
> +static struct edid *__ti_sn_bridge_get_edid(struct ti_sn_bridge *pdata,
> +                                           struct drm_connector *connector)
> +{
> +       struct edid *edid;
> +
> +       pm_runtime_get_sync(pdata->dev);
> +       edid = drm_get_edid(connector, &pdata->aux.ddc);
> +       pm_runtime_put(pdata->dev);

As mentioned in my patch [1], the above is a bit iffy for eDP.
Specifically just doing a pm_runtime_get_sync() isn't enough to
actually be able to read the EDID. We also need to do any panel power
sequencing and potentially set the "ignore HPD" bit in the bridge to
actually read the EDID.

I'll try to find some time to make this better--let's see if I get
distracted before I manage it.


> +
> +       return edid;
> +}
> +
>  /* -----------------------------------------------------------------------------
>   * DRM Connector Operations
>   */
> @@ -277,11 +289,8 @@ static int ti_sn_bridge_connector_get_modes(struct drm_connector *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)
> +               edid = pdata->edid = __ti_sn_bridge_get_edid(pdata, connector);

It feels weird to me that we are now exposing the get_edid() function
directly but we still need to implement get_modes(). I guess this is
because we might need to fallback to the hardcoded modes? ...but that
seems like it would be a common pattern for eDP bridges...


>         if (edid && drm_edid_is_valid(edid)) {
>                 ret = drm_connector_update_edid_property(connector, edid);
> @@ -871,12 +880,21 @@ static void ti_sn_bridge_post_disable(struct drm_bridge *bridge)
>         pm_runtime_put_sync(pdata->dev);
>  }
>
> +static struct edid *ti_sn_bridge_get_edid(struct drm_bridge *bridge,
> +                                         struct drm_connector *connector)
> +{
> +       struct ti_sn_bridge *pdata = bridge_to_ti_sn_bridge(bridge);
> +
> +       return __ti_sn_bridge_get_edid(pdata, connector);
> +}
> +
>  static const struct drm_bridge_funcs ti_sn_bridge_funcs = {
>         .attach = ti_sn_bridge_attach,
>         .pre_enable = ti_sn_bridge_pre_enable,
>         .enable = ti_sn_bridge_enable,
>         .disable = ti_sn_bridge_disable,
>         .post_disable = ti_sn_bridge_post_disable,
> +       .get_edid = ti_sn_bridge_get_edid,
>  };
>
>  /* -----------------------------------------------------------------------------
> @@ -1335,6 +1353,8 @@ static int ti_sn_bridge_probe(struct i2c_client *client,
>
>         pdata->bridge.funcs = &ti_sn_bridge_funcs;
>         pdata->bridge.of_node = client->dev.of_node;
> +       pdata->bridge.ops = DRM_BRIDGE_OP_EDID;
> +       pdata->bridge.type = DRM_MODE_CONNECTOR_eDP;

Even with the few comments above, I think this is still fine to move
us in the right direction. Unless I manage to fix up the EDID reading
(in which case your patch would conflict and would need to be
tweaked), then:

Reviewed-by: Douglas Anderson <dianders@chromium.org>


[1] https://lore.kernel.org/r/20210304155144.3.I60a7fb23ce4589006bc95c64ab8d15c74b876e68@changeid/
Laurent Pinchart March 26, 2021, 1:40 a.m. UTC | #3
Hi Doug,

On Wed, Mar 24, 2021 at 03:46:28PM -0700, Doug Anderson wrote:
> On Sun, Mar 21, 2021 at 8:02 PM Laurent Pinchart wrote:
> >
> > Implement the bridge connector-related .get_edid() operation, and report
> > the related bridge capabilities and type. The .get_edid() operation is
> > implemented with the same backend as the EDID retrieval from the
> > connector .get_modes() operation.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > ---
> >  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 30 ++++++++++++++++++++++-----
> >  1 file changed, 25 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > index dc300fab4319..6f6e075544e8 100644
> > --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > @@ -261,6 +261,18 @@ static void ti_sn_debugfs_remove(struct ti_sn_bridge *pdata)
> >         pdata->debugfs = NULL;
> >  }
> >
> > +static struct edid *__ti_sn_bridge_get_edid(struct ti_sn_bridge *pdata,
> > +                                           struct drm_connector *connector)
> > +{
> > +       struct edid *edid;
> > +
> > +       pm_runtime_get_sync(pdata->dev);
> > +       edid = drm_get_edid(connector, &pdata->aux.ddc);
> > +       pm_runtime_put(pdata->dev);
> 
> As mentioned in my patch [1], the above is a bit iffy for eDP.
> Specifically just doing a pm_runtime_get_sync() isn't enough to
> actually be able to read the EDID. We also need to do any panel power
> sequencing and potentially set the "ignore HPD" bit in the bridge to
> actually read the EDID.
> 
> I'll try to find some time to make this better--let's see if I get
> distracted before I manage it.

I've replied to your review of 05/11 with a possible solution. Fingers
crossed :-)

> > +
> > +       return edid;
> > +}
> > +
> >  /* -----------------------------------------------------------------------------
> >   * DRM Connector Operations
> >   */
> > @@ -277,11 +289,8 @@ static int ti_sn_bridge_connector_get_modes(struct drm_connector *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)
> > +               edid = pdata->edid = __ti_sn_bridge_get_edid(pdata, connector);
> 
> It feels weird to me that we are now exposing the get_edid() function
> directly but we still need to implement get_modes(). I guess this is
> because we might need to fallback to the hardcoded modes? ...but that
> seems like it would be a common pattern for eDP bridges...

Bridges are moving from creating the connector internally to providing a
set of bridge operations to support connector creation externally (by
the drm_bridge_connector helper, or by display drivers directly if
needed). During the transition, both need to be implemented, hence the
bridge .get_edid() operation for the new model, and the connector
.get_modes() operation for the old model.

> >         if (edid && drm_edid_is_valid(edid)) {
> >                 ret = drm_connector_update_edid_property(connector, edid);
> > @@ -871,12 +880,21 @@ static void ti_sn_bridge_post_disable(struct drm_bridge *bridge)
> >         pm_runtime_put_sync(pdata->dev);
> >  }
> >
> > +static struct edid *ti_sn_bridge_get_edid(struct drm_bridge *bridge,
> > +                                         struct drm_connector *connector)
> > +{
> > +       struct ti_sn_bridge *pdata = bridge_to_ti_sn_bridge(bridge);
> > +
> > +       return __ti_sn_bridge_get_edid(pdata, connector);
> > +}
> > +
> >  static const struct drm_bridge_funcs ti_sn_bridge_funcs = {
> >         .attach = ti_sn_bridge_attach,
> >         .pre_enable = ti_sn_bridge_pre_enable,
> >         .enable = ti_sn_bridge_enable,
> >         .disable = ti_sn_bridge_disable,
> >         .post_disable = ti_sn_bridge_post_disable,
> > +       .get_edid = ti_sn_bridge_get_edid,
> >  };
> >
> >  /* -----------------------------------------------------------------------------
> > @@ -1335,6 +1353,8 @@ static int ti_sn_bridge_probe(struct i2c_client *client,
> >
> >         pdata->bridge.funcs = &ti_sn_bridge_funcs;
> >         pdata->bridge.of_node = client->dev.of_node;
> > +       pdata->bridge.ops = DRM_BRIDGE_OP_EDID;
> > +       pdata->bridge.type = DRM_MODE_CONNECTOR_eDP;
> 
> Even with the few comments above, I think this is still fine to move
> us in the right direction. Unless I manage to fix up the EDID reading
> (in which case your patch would conflict and would need to be
> tweaked), then:
> 
> Reviewed-by: Douglas Anderson <dianders@chromium.org>
> 
> 
> [1] https://lore.kernel.org/r/20210304155144.3.I60a7fb23ce4589006bc95c64ab8d15c74b876e68@changeid/
diff mbox series

Patch

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index dc300fab4319..6f6e075544e8 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -261,6 +261,18 @@  static void ti_sn_debugfs_remove(struct ti_sn_bridge *pdata)
 	pdata->debugfs = NULL;
 }
 
+static struct edid *__ti_sn_bridge_get_edid(struct ti_sn_bridge *pdata,
+					    struct drm_connector *connector)
+{
+	struct edid *edid;
+
+	pm_runtime_get_sync(pdata->dev);
+	edid = drm_get_edid(connector, &pdata->aux.ddc);
+	pm_runtime_put(pdata->dev);
+
+	return edid;
+}
+
 /* -----------------------------------------------------------------------------
  * DRM Connector Operations
  */
@@ -277,11 +289,8 @@  static int ti_sn_bridge_connector_get_modes(struct drm_connector *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)
+		edid = pdata->edid = __ti_sn_bridge_get_edid(pdata, connector);
 
 	if (edid && drm_edid_is_valid(edid)) {
 		ret = drm_connector_update_edid_property(connector, edid);
@@ -871,12 +880,21 @@  static void ti_sn_bridge_post_disable(struct drm_bridge *bridge)
 	pm_runtime_put_sync(pdata->dev);
 }
 
+static struct edid *ti_sn_bridge_get_edid(struct drm_bridge *bridge,
+					  struct drm_connector *connector)
+{
+	struct ti_sn_bridge *pdata = bridge_to_ti_sn_bridge(bridge);
+
+	return __ti_sn_bridge_get_edid(pdata, connector);
+}
+
 static const struct drm_bridge_funcs ti_sn_bridge_funcs = {
 	.attach = ti_sn_bridge_attach,
 	.pre_enable = ti_sn_bridge_pre_enable,
 	.enable = ti_sn_bridge_enable,
 	.disable = ti_sn_bridge_disable,
 	.post_disable = ti_sn_bridge_post_disable,
+	.get_edid = ti_sn_bridge_get_edid,
 };
 
 /* -----------------------------------------------------------------------------
@@ -1335,6 +1353,8 @@  static int ti_sn_bridge_probe(struct i2c_client *client,
 
 	pdata->bridge.funcs = &ti_sn_bridge_funcs;
 	pdata->bridge.of_node = client->dev.of_node;
+	pdata->bridge.ops = DRM_BRIDGE_OP_EDID;
+	pdata->bridge.type = DRM_MODE_CONNECTOR_eDP;
 
 	drm_bridge_add(&pdata->bridge);