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

[Laurent Pinchart]

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

-- 
Regards,

Laurent Pinchart