On 29 September 2014 15:47, Thierry Reding [off-list ref] wrote:

On Mon, Sep 29, 2014 at 01:46:43PM +0200, Maxime Ripard wrote:

quoted

On Mon, Sep 29, 2014 at 12:18:08PM +0200, Thierry Reding wrote:

quoted

On Mon, Sep 29, 2014 at 11:23:01AM +0200, Maxime Ripard wrote:

quoted

On Mon, Sep 29, 2014 at 10:06:39AM +0200, Thierry Reding wrote:

[...]

quoted

quoted

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.

quoted

quoted

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!

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.

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.

quoted

quoted

quoted

quoted

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.

quoted

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.

And there is still the use of simplefb during development and for
generic distribution kernels. 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.

get you working display. And that's all it is. If you want to do it
properly go and write a DRM/KMS driver.

quoted

quoted

quoted

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?

Thanks

Michal