| Message ID | 20210322030128.2283-9-laurent.pinchart+renesas@ideasonboard.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/bridge: ti-sn65dsi86: Support DisplayPort mode | expand |
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>
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/
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 --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);
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(-)