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@xxxxxxxxxxxxxxxx>
> >>> ---
> >>> 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.
--
Regards,
Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
