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.I60a7fb23ce4589006bc95c64ab8d15c7...