Re: [RFC PATCH 2/3] drm: bridge: panel: allow override of the bus format

[Laurent Pinchart]

Hi Peter,

(CC'ing Jacopo Mondi)

On Sunday, 25 March 2018 15:01:11 EEST Peter Rosin wrote:
> On 2018-03-20 14:56, Laurent Pinchart wrote:
> > Hi Peter,
> > 
> > Thank you for the patch.
> > 
> > On Sunday, 18 March 2018 00:15:24 EET Peter Rosin wrote:
> >> Useful if the bridge does some kind of conversion of the bus format.
> >> 
> >> Signed-off-by: Peter Rosin <peda@xxxxxxxxxx>
> >> ---
> >> 
> >>  drivers/gpu/drm/bridge/panel.c | 22 +++++++++++++++++++++-
> >>  include/drm/drm_bridge.h       |  1 +
> >>  2 files changed, 22 insertions(+), 1 deletion(-)
> >> 
> >> diff --git a/drivers/gpu/drm/bridge/panel.c
> >> b/drivers/gpu/drm/bridge/panel.c index 6d99d4a3beb3..ccef0283ff41 100644
> >> --- a/drivers/gpu/drm/bridge/panel.c
> >> +++ b/drivers/gpu/drm/bridge/panel.c
> >> @@ -22,6 +22,7 @@ struct panel_bridge {
> >>  	struct drm_connector connector;
> >>  	struct drm_panel *panel;
> >>  	u32 connector_type;
> >> +	u32 bus_format;
> >>  };
> >>  
> >>  static inline struct panel_bridge *
> >> @@ -40,8 +41,15 @@ static int panel_bridge_connector_get_modes(struct
> >> drm_connector *connector) {
> >>  	struct panel_bridge *panel_bridge =
> >>  		drm_connector_to_panel_bridge(connector);
> >> 
> >> +	int ret;
> >> +
> >> +	ret = drm_panel_get_modes(panel_bridge->panel);
> >> +
> >> +	if (panel_bridge->bus_format)
> >> +		drm_display_info_set_bus_formats(&connector->display_info,
> >> +						 &panel_bridge->bus_format, 1);
> > 
> > While I agree with the problem statement and, to some extent, the DT
> > bindings, I don't think this is the right implementation. You've
> > correctly noted that display controller shouldn't blindly use the formats
> > reported by the panel through the connector formats, and that hacking the
> > panel driver to override the formats isn't a good idea, so I wouldn't
> > override the formats reported by the connector. I would instead extend
> > the drm_bridge API to report formats at bridge inputs. This would be more
> > generic and allow each bridge to configure itself according to the next
> > bridge in the chain.
> > 
> > I'm not sure whether this API extension should be in the form of a new
> > bridge function, or if the formats should be stored in the drm_bridge
> > structure directly as done for connectors in the display info structure.
> > I'm tempted by the former, but I'm open to discussions.
> 
> Ok, I can look into that, but let me check if I got this right. From the
> very little of the code that I have looked at, I have gathered that display
> controllers handle bridges explicitly, right?

That's correct, yes. Or, rather, they handle the first bridge in the chain, 
and then other bridges are handled recursively.

> If so, by extending the bridge (with either a new function or new data) you
> impose changes to all display controllers wanting to handle this new bridge
> input-format. If so, I assume I can leave out the changes to all display
> controllers that I do not care about. Correct?

That's correct.

> Also, don't hold your breath waiting for a v2, but I'll try to get to it :-)

I won't hold my breath, but Jacopo might :-) He has a similar issue to solve 
(reporting the LVDS modes supported by the bridge).

> >> -	return drm_panel_get_modes(panel_bridge->panel);
> >> +	return ret;
> >>  }
> >>  
> >>  static const struct drm_connector_helper_funcs
> >> @@ -203,6 +211,18 @@ void drm_panel_bridge_remove(struct drm_bridge
> >> *bridge)
> >>  }
> >>  EXPORT_SYMBOL(drm_panel_bridge_remove);
> >> 
> >> +void drm_panel_bridge_set_bus_format(struct drm_bridge *bridge, u32
> >> bus_format)
> >> +{
> >> +	struct panel_bridge *panel_bridge;
> >> +
> >> +	if (!bridge)
> >> +		return;
> >> +
> >> +	panel_bridge = drm_bridge_to_panel_bridge(bridge);
> >> +	panel_bridge->bus_format = bus_format;
> >> +}
> >> +EXPORT_SYMBOL(drm_panel_bridge_set_bus_format);
> >> +
> >>  static void devm_drm_panel_bridge_release(struct device *dev, void *res)
> >>  {
> >>  	struct drm_bridge **bridge = res;
> >> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> >> index 682d01ba920c..81903b92f187 100644
> >> --- a/include/drm/drm_bridge.h
> >> +++ b/include/drm/drm_bridge.h
> >> @@ -268,6 +268,7 @@ void drm_bridge_enable(struct drm_bridge *bridge);
> >>  struct drm_bridge *drm_panel_bridge_add(struct drm_panel *panel,
> >>  					u32 connector_type);
> >>  void drm_panel_bridge_remove(struct drm_bridge *bridge);
> >> +void drm_panel_bridge_set_bus_format(struct drm_bridge *bridge, u32
> >> bus_format);
> >> struct drm_bridge *devm_drm_panel_bridge_add(struct device *dev,
> >>  					     struct drm_panel *panel,
> >>  					     u32 connector_type);

-- 
Regards,

Laurent Pinchart