[linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code

Mon Sep 29 08:49:04 PDT 2014

On Mon, Sep 29, 2014 at 05:04:32PM +0200, Michal Suchanek wrote:
> On 29 September 2014 15:47, Thierry Reding <thierry.reding at gmail.com> wrote:
> > On Mon, Sep 29, 2014 at 01:46:43PM +0200, Maxime Ripard wrote:
> >> On Mon, Sep 29, 2014 at 12:18:08PM +0200, Thierry Reding wrote:
> >> > On Mon, Sep 29, 2014 at 11:23:01AM +0200, Maxime Ripard wrote:
> >> > > On Mon, Sep 29, 2014 at 10:06:39AM +0200, Thierry Reding wrote:
> >> > [...]
> >> > > > simplefb doesn't deal at all with hardware details. It simply uses what
> >> > > > firmware has set up, which is the only reason why it will work for many
> >> > > > people. What is passed in via its device tree node is the minimum amount
> >> > > > of information needed to draw something into the framebuffer. Also note
> >> > > > that the simplefb device tree node is not statically added to a DTS file
> >> > > > but needs to be dynamically generated by firmware at runtime.
> >> > >
> >> > > Which makes the whole even simpler, since the firmware already knows
> >> > > all about which clocks it had to enable.
> >> >
> >> > It makes things very complicated in the firmware because it now needs to
> >> > be able to generate DTB content that we would otherwise be able to do
> >> > much easier with a text editor.
> >>
> >> Didn't you just say that it was dynamically generated at runtime? So
> >> we can just ignore the "text editor" case.
> >
> > Perhaps read the sentence again. I said "that we would *otherwise* be
> > able to do much easier with a text editor.".
> >
> > My point remains that there shouldn't be a need to generate DTB content
> > of this complexity at all.
> >
> >> > Why do you think what I proposed isn't going to work or be reliable?
> >> > I don't see any arguments in the thread that would imply that.
> >>
> >> The fact that it broke in the first place?
> >
> > That's exactly the point. And it's going to break again and again as
> > simplefb is extended with new things. Generally DT bindings should be
> > backwards compatible. When extended they should provide a way to fall
> > back to a reasonable default. There's simply no way you can do that
> > with simplefb.
> >
> > What happened in the Snow example is that regulators that were
> > previously on would all of a sudden be automatically disabled on boot
> > because there was now a driver that registered them with a generic
> > framework.
> 
> So what? You get a driver for regulators and suddenly find that
> nothing registered regulators because they were on all the time anyway
> and everything breaks? What a surprise!

I agree, this is not a surprise at all. But like I pointed out this is
bound to happen again and again. So how about we learn from it and add
the functionality needed to prevent it from happening?

> > The same thing is going to happen with simplefb for your device. If you
> > later realize that you need a regulator to keep the panel going, you'll
> > have to add code to your firmware to populate the corresponding
> > properties, otherwise the regulator will end up unused and will be
> > automatically disabled. At the same time you're going to break upstream
> > for all users of your old firmware because it doesn't add that property
> > yet.
> 
> Sure. And what can you do about that? It's not like the original Snow
> firmware writes anything of use to the DT at all. So to run a
> development kernel you need a development firmware. If you add new
> features to the kernel that involve intefacing to the firmware you
> need to update the firmware as well. Once support for Snow platform is
> stable you can expect that users can use a stable release of firmware
> indefinitely.

Feel free to take that up with the DT ABI stability committee.

> > And the same will continue to happen for every new type of resource
> > you're going to add.
> 
> The like 5 resource types, yes. Some of which may not even apply to simplefb.

Today it's 5, tomorrow 6, next year, who knows.

> >> > > > So how about instead of requiring resources to be explicitly claimed we
> >> > > > introduce something like the below patch? The intention being to give
> >> > > > "firmware device" drivers a way of signalling to the clock framework
> >> > > > that they need rely on clocks set up by firmware and when they no longer
> >> > > > need them. This implements essentially what Mark (CC'ing again on this
> >> > > > subthread) suggested earlier in this thread. Basically, it will allow
> >> > > > drivers to determine the time when unused clocks are really unused. It
> >> > > > will of course only work when used correctly by drivers. For the case of
> >> > > > simplefb I'd expect its .probe() implementation to call the new
> >> > > > clk_ignore_unused() function and once it has handed over control of the
> >> > > > display hardware to the real driver it can call clk_unignore_unused() to
> >> > > > signal that all unused clocks that it cares about have now been claimed.
> >> > > > This is "reference counted" and can therefore be used by more than a
> >> > > > single driver if necessary. Similar functionality could be added for
> >> > > > other resource subsystems as needed.
> >> > >
> >> > > So, just to be clear, instead of doing a generic clk_get and
> >> > > clk_prepare_enable, you're willing to do a just as much generic
> >> > > clk_ignore_unused call?
> >> >
> >> > Yes.
> >> >
> >> > > How is that less generic?
> >> >
> >> > It's more generic. That's the whole point.
> >> >
> >> > The difference is that with the solution I proposed we don't have to
> >> > keep track of all the resources. We know that firmware has set them up
> >> > and we know that a real driver will properly take them over at some
> >> > point
> >>
> >> You keep saying that... and you know that you can't make this
> >> assumption.
> >
> > Why not? Are you really expecting to keep running with simplefb forever?
> > Nobody is going to seriously use an upstream kernel in any product with
> > only simplefb as a framebuffer. I've said before that this is a hack to
> 
> Why not? You can use shadowfb (software acceleration) with simplefb
> all right. Shadowfb is hands down the fastest video acceleration we
> have on anything but hardware that has _very_ slow CPU or that can run
> Intel UXA drivers. With manufacturers adding more and more superfluous
> cores to the CPUs shadowfb is actually not too stressing on the
> system, either.
> 
> On hardware like Allwinner A13 (single core) the use of shadowfb over
> actual video acceleration hardware has its benefits and drawbacks and
> is neither clearly better nor worse. The same hardware tends to have
> only one fixed video output - a tablet LCD panel. On such hardware
> modesetting is needless luxury if u-boot provides the simplfb
> bindings, at least for some use cases.

I presume some of those tablets will have HDMI connectors. And surely
somebody will eventually put an Allwinner SoC into something that's not
a tablet.

Note also that with only simplefb you won't be able to use any kind of
hardware acceleration, not even 3D. Perhaps you could even pull it off
but certainly not with any kind of double-buffering, so while it might
be an easy way to get something to display, please stop pretending that
DRM/KMS don't matter.

Also this is all hypothetical since Luc said himself that he's already
working on a DRM/KMS driver.

> And there is still the use of simplefb during development and for
> generic distribution kernels.

Generic distribution kernels can use DRM/KMS just fine. And they also
want to use it because it gives them a lot of advanced features using a
standard API.

> I prefer that during development of the
> KMS driver and during bootup before the KMS module becomes available
> the system behaves normally, not in special hacked way that is useless
> for anything but this special system mode. Worse, such special mode
> will not be regularly tested on well supported hardware and will
> likely bitrot and lead to mysterious issues at boot time when KMS is
> not available and this special system mode is in effect.

It's not at all special or hacked. It merely uses a couple of
assumptions that differ from what you might expect from a regular
driver. And that's really the essence of simplefb.

> > get you working display. And that's all it is. If you want to do it
> > properly go and write a DRM/KMS driver.
> >
> >> > > You know that you are going to call that for regulator, reset, power
> >> > > domains, just as you would have needed to with the proper API, unless
> >> > > that with this kind of solution, you would have to modify *every*
> >> > > framework that might interact with any resource involved in getting
> >> > > simplefb running?
> >> >
> >> > We have to add handling for every kind of resource either way. Also if
> >> > this evolves into a common pattern we can easily wrap it up in a single
> >> > function call.
> >>
> >> Unless that in one case, we already have everything needed to handle
> >> everything properly, and in another, you keep hacking more and more
> >> into the involved frameworks.
> >
> > This is a fundamental issue that we are facing and I'm trying to come up
> > with a solution that is future-proof and will work for drivers other
> > than simplefb.
> 
> How is proper resource management not going to work for drivers other
> than simplefb?

I have no doubt that it's going to work. It already does. But unless we
solve this at the core we'll be reiterating this for every new type of
resource that we need to support and for every other driver of the same
nature (i.e. uses a "virtual" device set up by firmware).

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140929/d541b228/attachment-0001.sig>