On Fri, May 09, 2014 at 09:59:34AM -0400, Rob Clark wrote:
> On Fri, May 9, 2014 at 5:08 AM, Andrzej Hajda <a.hajda@xxxxxxxxxxx> wrote:
[...]
> > 4. And the last thing, it is more about the concept/design. drm_bridge,
> > drm_hw_block suggests that those interfaces describes the whole device:
> > bridge, panel, whatever.
>
> hmm, I don't think this is the case. I can easily see things like:
>
> struct foo_panel {
> struct drm_panel base;
> struct drm_bridge bridge;
> ...
> }
>
> where a panel implementation implements both panel and bridge. In
> fact that is kinda what I was encouraging.
For lack of a better entry point into the discussion, let me pick this
example. It seems like in general we all agree that the basic structure
would have to be something like this:
CRTC -> encoder -> bridge [ -> bridge ... ] -> panel
Where panel could optionally be a bridge/panel hybrid as Rob pointed
out. I'm not sure if there's panels like this, but I suspect the use
case would be where a panel had built-in controls to modify the image in
some fashion (brightness, saturation, ...). I think those things exist
in DCS for example.
What's missing here, and if I understand correctly what Sean said about
the connector type, what we need to solve is how to wire up the
connector so that it reflects the correct type. So the above would have
to become something like this:
CRTC -> encoder -> bridge [ -> bridge ... ] -> panel
connector -----------------------------^
One of the problems with that approach is that panel is more than just a
video sink. It also controls the panel (and optionally backlight) power.
The standard DRM connector helpers don't work well in that case, because
drm_helper_connector_dpms() forwards changes to the CRTC and encoder,
though that could possibly be solved by wrapping it in a small custom
implementation that also controls the panel. Anyway, that's really just
an implementation detail.
So once a complete chain from encoder to panel has been created, we need
a way to find the appropriate connector type. Perhaps to achieve that it
would be useful to instantiate the connector by the bridge that's
connected to the panel. So essentially something like this:
struct drm_bridge {
/* to allow chaining */
struct drm_bridge *next;
/* for bridges directly connected to a panel or monitor */
struct drm_connector *connector;
/* for bridges directly connected to a panel */
struct drm_panel *panel;
...
};
To make this work in a generic way, I think we're missing one bridge
instance. Currently the bridge in an encoder is completely optional, so
if there is no bridge in the system this needs to be special cased. One
way to unify this would be to make drm_encoder implement drm_bridge, or
an alternative would be to make drm_panel implement a bridge. Both can
possibly be dummies stubbed out with simple helpers.
I wonder if this would have any consequences on Dave's DP MST work,
since presumably an MST hub could be considered a bridge. In that case I
guess there would need to be support for multiple "next" bridges,
perhaps a simple doubly-linked list instead of a next pointer. The first
bridge would then be used to model the hub's input and child bridges
would be used to model each
of the outputs.
Thierry
Attachment:
pgp04g7a0cVO5.pgp
Description: PGP signature
