Hi Boris,
On Thu, Aug 22, 2019 at 08:02:47PM +0200, Boris Brezillon wrote:
On Thu, 22 Aug 2019 19:35:24 +0300 Laurent Pinchart wrote:
On Thu, Aug 22, 2019 at 06:29:09PM +0200, Boris Brezillon wrote:
On Tue, 20 Aug 2019 04:16:44 +0300 Laurent Pinchart wrote:
Implement the newly added bridge connector operations, allowing the usage of drm_bridge_panel with drm_bridge_connector.
Signed-off-by: Laurent Pinchart laurent.pinchart@ideasonboard.com
drivers/gpu/drm/bridge/panel.c | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c index f5b8e55301ac..1c7f5b648f05 100644 --- a/drivers/gpu/drm/bridge/panel.c +++ b/drivers/gpu/drm/bridge/panel.c @@ -60,7 +60,7 @@ static int panel_bridge_attach(struct drm_bridge *bridge, int ret;
if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)
return -EINVAL;
return 0;if (!bridge->encoder) { DRM_ERROR("Missing encoder\n");
@@ -123,6 +123,18 @@ static void panel_bridge_post_disable(struct drm_bridge *bridge) drm_panel_unprepare(panel_bridge->panel); }
+static int panel_bridge_get_modes(struct drm_bridge *bridge,
struct drm_connector *connector)+{
- struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge);
- /*
* FIXME: drm_panel_get_modes() should take the connector as an* argument.*/- return drm_panel_get_modes(panel_bridge->panel);
+}
static const struct drm_bridge_funcs panel_bridge_bridge_funcs = { .attach = panel_bridge_attach, .detach = panel_bridge_detach, @@ -130,6 +142,7 @@ static const struct drm_bridge_funcs panel_bridge_bridge_funcs = { .enable = panel_bridge_enable, .disable = panel_bridge_disable, .post_disable = panel_bridge_post_disable,
- .get_modes = panel_bridge_get_modes,
};
/** @@ -175,6 +188,9 @@ struct drm_bridge *drm_panel_bridge_add(struct drm_panel *panel, #ifdef CONFIG_OF panel_bridge->bridge.of_node = panel->dev->of_node; #endif
- panel_bridge->bridge.ops = DRM_BRIDGE_OP_MODES;
- /* FIXME: The panel should report its type. */
- panel_bridge->bridge.type = DRM_MODE_CONNECTOR_DPI;
Shouldn't we patch all panel drivers to expose this type before doing this change? I mean, the connector type is exposed to userspace, and I wouldn't be surprised if some userspace apps/libs decided to base their output selection logic on this field.
Note that this type will only make it to userspace for drivers that use the bridge->type field, likely through the drm bridge connector helper. I do agree that panel drivers should be updated, but given the number of panels in panel-simple and the fact that the information would need to be researched for most of them, this will be significant work. Can't this be done when converting display controller drivers on a need basis ?
I think setting a default value and fixing things on a need basis is okay, but that doesn't prevent you from adding the necessary infrastructure to let panel drivers pass this type (we can fallback to a default type in panel drivers instead of here).
I'll add the infrastructure/
I'm also not sure why 'DPI' is chosen as the default, shouldn't we use 'Unknown' instead?
Mostly to avoid breaking userspace, as most panels supported by drm_panel use DPI.
Or maybe we could, as an interim measure, derive the type from the bus formats reported by the panel if the panel type is not set ? If the panel reports MEDIA_BUS_FMT_RGB666_1X7X3_SPWG, MEDIA_BUS_FMT_RGB666_1X7X4_SPWG or MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA then we can set the type to LVDS, otherwise we set it to DPI.
Hm, aren't we better off patching panel descs exposing these bus formats to also explicitly set desc->type to LVDS, leaving others to Unknown (Unknown is 0, so you don't have to patch all panel_desc definitions)?
I was thinking about adding this logic to drivers/gpu/drm/bridge/panel.c, which would avoid patching lots of panel drivers as a prerequisite. With such a logic there, plus a default to DPI, I thought we would be good enough for an initial version. It would leave DSI unhandled, so maybe not the best :-S
I can submit a patch to add a type field to the panel structure and implement this logic.