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

[Laurent Pinchart]

Hi Jacopo,

On Tuesday, 27 March 2018 11:16:01 EEST jacopo mondi wrote:
> On Mon, Mar 26, 2018 at 10:03:31PM +0300, Laurent Pinchart wrote:
> > On Sunday, 25 March 2018 15:01:11 EEST Peter Rosin wrote:
> >> On 2018-03-20 14:56, Laurent Pinchart wrote:
> >>> 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).
> 
> I was not :) I jumped late on this, as I restarted the DRM bridge work
> yesterday. Peter, can I summarize you my current use case? (which is
> quite similar to yours actually)
> 
> At the R-Car DU (Display Unit) output, we have an LVDS encoder
> connected to a 'transparent' LVDS bridge that converts the input LVDS
> stream into digital RGB output to be then HDMI encoded by another
> component.
> 
> The 'transparent LVDS decoder', for which I'm now writing a driver,
> should report what pixel bus format it accepts as input as the DU LVDS
> encoder can output an LVDS stream with several different component ordering
> schemes. I was about to come up with a proposal last week but you beat
> me at time, so I'm happy to base my work on what comes out from this
> series.
> 
> ---- Laurent: On the THC63LVD driver
> Laurent: at the same time I would not block the THC63LVD1024 driver. I
> can extend bindings to include the "mode map" configuration property,
> and squash my Eagle DTS patch on top of Niklas' one. Then make the newly
> introduced driver use whatever API comes out from this series with an
> incremental patch. Does this work for you? It is true, though, that
> we're anyway late for v4.17 and I could send one single series based
> on some future version of this and that includes all (bridge driver
> and bindings, DU LVDS changes and Eagle DTS), but I feel it's easier
> if we got the bridge driver accepted first, and then develop on top of
> that.

I'm fine with both options, you can pick the one that will match what is ready 
in upstream by the time you get to submit the patches.

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