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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Rob and Sascha,

On Wednesday 21 August 2013 08:22:59 Rob Clark wrote:
> On Wed, Aug 21, 2013 at 3:09 AM, Sascha Hauer 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 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?

Many chips and IP cores in KMS and V4L2 pipelines are pretty simple, and the 
bulk of the driver is just an API implementation. Register poking is usually a 
small percentage of the code (that's of course not the case for the more 
complex IP cores, but those are usually not shared).

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

The point is I believe they will become less and less exceptional over time. 
One might argue that CDF is a revolutionary approach as opposed to an 
evolutionary approach, but looking at the future I believe we'll need a much 
bigger disruption if we wait too long.

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

Don't get me wrong, drm_bridge is an interesting idea, but I think it's a bit 
limited.

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

At least we agree on that, good :-)

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

I understand. But DRM drivers already depend on lots of infrastructure code 
that is not part of DRM (you could of course argue that it's not a valid 
reason to add one more :-)).

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

I very much like the KMS property API (there's of course always room for 
improvements), we're actually thinking about a property API for V4L2 as well. 
My concern is that we need something shareable between subsystems, and the KMS 
version is a bit too tied to DRM/KMS for that purpose. For instance struct 
drm_property inherits struct drm_mode_object, and thus requires a DRM device 
to allocate IDs. This is the kind of really low-level issues that we will need 
to sort out one way or the other. If we can't require a V4L2 driver to 
implement a drm_device, or the other around (and I believe it would be 
unreasonable to do so), we will need a neutral middle-ground.

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

I absolutely agree that we should not repeat that mistake. We hopefully have a 
bit more experience now, and we should catch this kind of issue during code 
review in the worst case.

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

:-D

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

I pretty much agree, DT bindings won't depend on whether we upstream the code 
to drivers/video/, drivers/gpu/ or drivers/media/. However, it's easy to 
design DT bindings with a particular framework implementation in mind and use 
shortcuts that make the bindings difficult to use should a different framework 
be implemented. We need to be careful about this. On the upside we now have 
more experienced DT bindings reviewers than a couple of years ago, I'm thus 
pretty optimistic.

-- 
Regards,

Laurent Pinchart