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

[Marcel Ziswiler]

On 21 Aug 2015 11:29, Thierry Reding <treding at nvidia.com> wrote:

> I agree with Stephen that U-Boot should use an exact copy of the DTS
> files in the kernel. The bits in the kernel get much more review and
> have been known to work well for a number of years.

I do not oppose however I doubt this has ever been the case. At least for the Tegras this is far from the truth.

> U-Boot depending on the device tree or not is really an orthogonal
> issue. If we keep a delta between U-Boot and Linux DTS files, we'll risk
> breaking things badly if we ever do end up putting device tree sources
> into a common source tree, because we'd be exposing U-Boot to something
> that it wasn't tested against.

I'm not even that sure whether using device trees in U-Boot is really that smart at all especially considering SPL and such. But that's a different thematic...

> That's certainly a true statement. There really should never have been
> such a disconnect as we currently have. If we can't agree on using the
> very same DTS files for both U-Boot and Linux we might as well not use
> device tree at all (in U-Boot).

Exactly.

> Perhaps a good idea would be to simply copy what we have in the kernel
> and see where (if at all) U-Boot breaks down and fix it to work properly
> with "upstream" DTBs.

I can certainly give this a try on our hardware however most NVIDIA boards are far or at least were far from public so I can't test those as I don't have any access.

> I can't obviously force you to do all that work to fix past mistakes,
> but I'd like to see at least new DTS content in U-Boot deviate from what
> we have upstream in the kernel.

OK, fair enough.

> It looks to me like this adds even a third copy. There's already a
> duplicate of the display controller bits in drivers/video/tegra124 along
> with some new code to support eDP.

Me wondering how that came along then?

> There really isn't much reason for
> duplicating the drivers since the display controllers haven't changed
> very much over the various SoC generations. And especially in U-Boot I
> don't think you'll need fancy features like color conversion or gamma
> correction, or smart dimming, which are really the only areas where
> there have been changes.
>
> If you look at the Linux kernel driver we get away with very minimal
> parameterization, and I think the same should be possible for U-Boot.

Agreed, just wondering about the lack of any documentation thereof.

> In a similar vein I think it should be possible to write unified drivers
> for each type of output (RGB, HDMI, DSI, SOR). Those require even less
> parameterization since there have been almost no changes to those since
> their introduction, the rare exception being fancy features that I don't
> think would be needed for U-Boot.
>
> Granted, U-Boot doesn't give you much of a framework to work with, so
> there's a lot of code that would need to be written to abstract things,
> but I think that's effort well spent, much better than simply
> duplicating for each new generation.

As mentioned before my doubts go as deep as the actual functional split thereof.

> Frankly I think it should all move into a separate "tegra" subdirectory
> in drivers/video/. There's likely going to be quite a few files if we
> want to support several types of outputs, and a subdirectory will help
> keep things organized.
>
> I think a framework for U-Boot could be fairly simple and doesn't have
> to have the complexity of DRM/KMS in the kernel. I'd expect the U-Boot
> configuration to be statically defined, and if the "framework" is Tegra
> specific it doesn't need to be as generic either.
>
> Perhaps something as simple as:
>
>         struct tegra_dc {
>                 ...
>                 int (*enable)(struct tegra_dc *dc, const struct display_mode *mode);
>                 void (*disable)(struct tegra_dc *dc);
>                 ...
>         };
>
>         struct tegra_output {
>                 ...
>                 struct tegra_dc *dc;
>                 ...
>                 int (*enable)(struct tegra_output *output, const struct display_mode *mode);
>                 void (*disable)(struct tegra_output *output);
>                 ...
>         };
>
> would work fine. That's roughly how drivers are implemented in the
> kernel. Setting up display on an output would be done by determining the
> mode (typically by parsing EDID if available, or using a hard-coded mode
> otherwise) and then calling:
>
>         output->dc = dc;
>         dc->enable(dc, mode);
>         output->enable(output, mode);
>
> You might want to add in an abstraction for panels as well to make sure
> you have enough flexibility to enable and disable those, too. In that
> case you'd probably want to complement the above sequence with:
>
>         panel->enable(panel);
>
> 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.

I also doubt finding that kind of time. My goal was simply to make T30 usable to the same extend as the T20 before it gets completely lost.