On Thu, 22 Aug 2019 19:35:24 +0300
Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx> wrote:
> Hi Boris,
>
> 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'm also not sure why
'DPI' is chosen as the default, shouldn't we use 'Unknown' instead?
>
> 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 can submit a
> patch to add a type field to the panel structure and implement this
> logic.
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
