[U-Boot] [PATCH 3/5] arm: tegra30: video: integrate display driver for t30

[Simon Glass]

Hi Thierry,

On 26 August 2015 at 00:29, Thierry Reding <treding at nvidia.com> wrote:
> On Tue, Aug 25, 2015 at 10:03:13AM -0600, Simon Glass wrote:
>> On 25 August 2015 at 05:02, Thierry Reding <treding at nvidia.com> wrote:
>> > On Mon, Aug 24, 2015 at 10:58:48AM -0600, Simon Glass wrote:
>> >> On 24 August 2015 at 04:12, Thierry Reding <treding at nvidia.com> wrote:
> [...]
>> >> > SOR is an even worse abstraction because it's completely Tegra-specific
>> >> > and other SoCs will have completely different ways of providing the same
>> >> > types of output. You'll end up with a uclass containing a single
>> >> > implementation.
>> >>
>> >> But if it is a single implementation why do you need to add function
>> >> pointers? It would just be a normal call in that case. I'm not
>> >> suggesting we add uclasses with no generic use.
>> >
>> > The function pointers are there to allow the display driver to call into
>> > the different output drivers. Generally on Tegra what you do to scan out
>> > a framebuffer is roughly this:
>> >
>> >         1) setup display controller to drive a specified display mode
>> >         2) enable a window to scan out a given framebuffer
>> >         3) setup an output driver to take input from the display
>> >            controller and push it over the wire
>> >
>> > 1) and 2) will be the same no matter what output you use. 3) is specific
>> > to the type of output, but can be done with the same software interface.
>> >
>> > The function pointers help in implementing 3) using the same abstraction
>> > which I called tegra_output. A driver for HDMI would implement this in
>> > one way, while the driver for DSI would implement it in another. For SOR
>> > you would have yet another implementation. But the display driver itself
>> > would be able to treat them the same way.
>>
>> OK then I think we should create a new video output uclass for this
>> purpose. We then end up with a high-level 'interface' uclass (hdmi,
>> displayport, etc.) and a low-level 'video transport' uclass.
>
> I don't understand. This is all internal to the display driver, why
> should it need to bother with uclasses?

One of the design goals of driver model is to expose these interfaces
so that the structure of devices can be seen. It hopefully promotes
code reuse and better software architecture since more thought goes
into exactly where these interfaces should be drawn, and whether they
can be shared across different drivers for the same subsystem.

Driver model was created to expose and standardise the various
interfaces in U-Boot between generic code (or even arch-specific code)
and the drivers it uses. This doesn't just apply at the top level -
e.g. pinctrl is arguably something that could be dealt with internally
by each driver, but we choose to make it its own first-class
subsystem. For Ethernet we have a PHY library to promote code reuse.

I'm sure there are merits on each side but I think the default
position should be that whenever an interface with function methods is
added we should consider it a new uclass. This is particularly true
when we have a device tree child since we want to main that tree
structure in driver model so that nodes correspond to devices.

I'm sure there are exceptions where this does not make sense.

>
>> >> > So, to reiterate, the above wasn't meant to be a generic abstraction for
>> >> > a U-Boot-wide display framework, but rather a suggestion on how the
>> >> > Tegra driver could internally be structured in order to avoid code
>> >> > duplication.
>> >> >
>> >> >> > Which should work for everything, except maybe DSI, where you may need
>> >> >> > some sort of inbetween step for panels that need additional setup using
>> >> >> > DCS commands or the like. But I suspect that's a bridge that can be
>> >> >> > crossed when we get to it.
>> >> >> >
>> >> >> > That said, I don't forsee myself having any time to devote to this, but
>> >> >> > if anyone ends up spending work on this, feel free to Cc me on patches
>> >> >> > or ask if you have questions about the display hardware or the framework
>> >> >> > design. I'm sure I can find the time to provide feedback.
>> >> >>
>> >> >> In which case I suggest we limit the amount of rewrite we ask for in
>> >> >> this case...
>> >> >
>> >> > People asked for my opinion, so I shared. If you prefer code duplication
>> >> > over a properly architected driver that's of course your prerogative.
>> >>
>> >> I am wondering if the problem here is just that I misunderstood your
>> >> intent. How about:
>> >>
>> >> - the display controller code (display.c) should be common across all Tegra SoCs
>> >> - the code (which was merged 3 years ago) should move to use the new
>> >> device tree bindings (as does tegra124 display support)
>> >>
>> >> What am I missing?
>> >
>> > Sounds good to me. My suggestions were targetted at how to decouple some
>> > of the code to allow both the RGB (for the existing Tegra20 support) and
>> > SOR (for the existing Tegra124 support) outputs to be used, depending on
>> > the particular use-case.
>> >
>> > And yes, this can all be driven from DT. The driver should be able to
>> > parse the "primary" output from DT using information that's available
>> > (such as which outputs are enabled) and some heuristics in case multiple
>> > outputs are enabled (DSI or RGB and HDMI for example). In all cases that
>> > I know of the internal panel (RGB, DSI, eDP) will be the primary one, so
>> > giving those priority over HDMI should always give you a sane default
>> > configuration.
>> >
>> > Once you have the primary output you can query what mode it should drive
>> > (using static display mode information from an internal panel, or
>> > preferably EDID), allocate an appropriately sized framebuffer, set the
>> > mode and scan out that buffer.
>>
>> OK, sounds good. I'm not yet sure who is planning to work on this and
>> I would much prefer to do it with another SoC uses the same framework.
>> It's normally a bad idea to design a general purpose framework with a
>> sample size of 1!
>
> Again, this is about adding infrastructure to the Tegra display driver
> to allow code reuse. Why would this need to be done in conjunction with
> other SoCs?

It seems like the operations you are proposing would be the same on
any SoC with this hardware.

Regards,
Simon