Re: [PATCH/RFC v3 00/19] Common Display Framework

[Rob Clark <robdclark@gmail.com>]

On Wed, Aug 21, 2013 at 3:09 AM, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> On Tue, Aug 20, 2013 at 02:40:55PM -0400, Rob Clark wrote:
>> On Tue, Aug 20, 2013 at 11:24 AM, Laurent Pinchart
>> <laurent.pinchart@ideasonboard.com> wrote:
>> > Hi Rob,
>> >
>> >> Or maybe, put another way, I still think we should still optimize for the
>> >> common case. I mean I'm sure you *can* design hw that has an LVDS->DP bridge
>> >> in the capture path, and if you had something like an FPGA where that was
>> >> cheap to do maybe you even would (for fun). But if in the real world, there
>> >> are only one or two cases of actual hw using the same bridge in a capture
>> >> pipeline which is normally used in display pipelines, then duplicating some
>> >> small bit of code for that abnormal case if it makes things easier for the
>> >> common case, seems like a reasonable trade-off to me.
>> >
>> > That was my opinion as well until I started working on a Xilinx platform.
>> > There's dozens of IP cores there that are used in both capture and display
>> > pipelines.
>>
>> or maybe just some helper lib's to handling the register level programming?
>>
>> Anyways, I guess you can call it a "worse is better" thing, but if you
>> can get 90% of the desired effect for 10% of the work, take the
>> simpler solution.  I don't think we should make things more layered or
>> more difficult for one exceptional case.
>>
>>
>> > Furthermore, FPGA display pipelines being made of individual IP cores, we need
>> > a way to write one driver per IP core and make them all interact. The recently
>> > proposed drm_bridge is one possible solution to this issue (and to a different
>> > but similar issue that trigerred the development of drm_bridge), but it's in
>> > my opinion not generic enough. We're facing several problems that are related,
>> > it would really be a shame not to solve them with a good enough framework.
>>
>> well, I've been working on DSI panel support in msm drm, and also been
>> looking at the patches to add DSI support in i915.  And the panel
>> interface ends up looking basically like the drm_bridge interface.
>> Perhaps the only thing not generic there is the name.
>>
>> >> I mean, take a DSI panel driver, for example.. anything but a trivial panel
>> >> driver, you are going to want to expose some custom properties on the
>> >> connector for controlling non-standard features. So you end up with both
>> >> cdf_foo for the common part, and drm_foo for gluing in the properties. And
>> >> now these two parts which otherwise would be one, end up having to stay in
>> >> sync merged through different trees, etc. It seems a lot like making my life
>> >> more difficult for a fairly hypothetical gain ;-)
>> >
>> > The DSI panel driver should not be split into two parts. It should implement a
>> > CDF entity, any glue needed for DRM will not be part of the panel driver.
>>
>> right, but that glue ends up needing to be *somewhere*, which makes it
>> the 2nd part.
>>
>> >> Or, take an hdmi or DP bridge. To use any of the common
>> >> infrastructure/helpers in drm, you end up implementing a generic interface,
>> >> where both the producer and consumer is inside drm. Which just seems a bit
>> >> pointless and extra hoops to jump through. Especially when we discover that
>> >> we need to extend/enhance the common interface outside of drm to make it
>> >> work. (I'm thinking of display_entity_control_ops::get_modes() here, but I'm
>> >> sure there are more examples.) And I think we'll run into similar issues
>> >> with display_entity_control_ops::set_state(), since the on<->off sequencing
>> >> can get hairy when the upstream/downstream entity is fed a clk by the
>> >> downstream/upstream entity. And similarly, I think we'll go through a few
>> >> revisions of DSI panel/bus params before we have everything that everyone
>> >> needs.
>> >
>> > I don't see where needing multiple revisions of a patch set would be bad :-)
>>
>> I mean over multiple kernel revisions, as people start adding support
>> for new hw and run into limitations of the "framework".
>
> A framework can be changed, extended and fixed. In the end we talking
> about a completely in-kernel framework for which we do not have to
> maintain a stable API.

oh sure, this is why I'd be absolutely against exposing this to
userspace currently..

But my only point here was that an in-drm framework, all the fix-ups
due to a framework change are sorted out before Dave sends his pull
req to linus.  We do this on a semi-regular basis already in drm
already.

Anyways, not a show-stopper thing.. just (in my mind) one additional
inconvenience (and not really the biggest one) about having the
"framework" outside of drm.  I'm more concerned about just having the
"display entity" code at a layer below the helpers and property api's
that I'd like to use in drm.

And just to be clear, part of my negative experience about this is the
omapdss/omapdrm split.  I just see cfd outside of drm as encouraging
others to make the same mistake.

>>
>> We've already figured out that just having enable() and disable() is
>> not sufficient for displayport link training.  I'm not sure what else
>> we'll discover.
>
> It's pretty much expected that other things will be discovered, but this
> will also happen with all sub-drm-driver frameworks we currently have.

right, which is why I want to "optimize" for this case by having
everything effected by evolving the framework merged via drm.

>>
>> I'm not saying not to solve (most of these) problems.. I don't think
>> chaining multiple drm_bridge's is impossible, I can think of at least
>> two ways to implement it.  But I think we should solve that as someone
>> is adding support for hw that uses this.  This (a) lets us break
>> things up into smaller more incremental changes (drm_bridge is
>> *considerably* smaller than the current CDF patchset), and (b) avoids
>> us solving the wrong problems.
>>
>> btw, I think DT bindings is orthogonal to this discussion.
>
> Not really. The common display framework is about splitting the pipeline
> into its components and adding a common view to the components. It's CDF
> helper code that can translate a DT binding directly into a encoder
> pipeline.

yes, DT binding are orthogonal.  This is something that should work
for BIOS, ACPI, DT, fex (just kidding), etc.  I mean there might be
some DT helpers, but that should be kind of on the side.

If DT binding are really to be OS independent, we probably need some
better way to deal w/ collecting up DT nodes for one (userspace
visible) driver (or cases where driver <-> DT is not 1<->1).  But that
is really an entirely different topic, or at least not applicable to
where the "framework" lives.

BR,
-R

> Sascha
>
>
> --
> Pengutronix e.K.                           |                             |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |