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

[Michal Suchanek]

On 29 September 2014 17:49, Thierry Reding <thierry.reding at gmail.com> wrote:
> 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.

As pointed out you can also generate by hand a DT that exactly
describes what the preinstalled firmware sets up - so long as the
preinstalled firmware sets upa any usable framebuffer at all and
always sets up the same thing. If the preinstalled firmware can set up
the framebuffer in different ways and is not willing to communicate to
the kernel in what way it set up the framebuffer then it is unusable
with simplefb. That sucks but there is no way around that.

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

Nope. Only 1 output.

> somebody will eventually put an Allwinner SoC into something that's not
> a tablet.

Sure. And you might want to bother with modesetting then.

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

Nope. I won't be able to use that staggering performance of 1 hardware shader.

But my UI will draw really fast unless I want those wobbly windows.
Now who cares about wobbliness when you want to run everything
fullscreen anyway to get the most of the 480p screen estate.

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

Yes, it will matter for 3d and might matter for video acceleration as well.

But will not matter for using a terminal emulator or reading a book.

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

Once it's solved for every resource every 'virtual' driver will work
the same way.

I do not see a problem here.

Thanks

Michal