Hi,
On Sun, Mar 21, 2021 at 8:02 PM Laurent Pinchart
<laurent.pinchart+renesas@xxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxx>
> ---
> 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@xxxxxxxxxxxx>
[1] https://lore.kernel.org/r/20210304155144.3.I60a7fb23ce4589006bc95c64ab8d15c74b876e68@changeid/
