Re: [PATCH v2 13/50] drm/bridge: panel: Implement bridge connector operations

[Boris Brezillon <boris.brezillon@collabora.com>]

On Thu, 22 Aug 2019 19:35:24 +0300
Laurent Pinchart <laurent.pinchart@ideasonboard.com> 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@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'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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel