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

[Simon Glass]

Hi,

On 21 August 2015 at 03:27, Thierry Reding <treding at nvidia.com> wrote:
> On Thu, Aug 20, 2015 at 11:46:41PM +0000, Marcel Ziswiler wrote:
>> On 20 Aug 2015 22:09, Stephen Warren <swarren at wwwdotorg.org> wrote:
>>
>> > Hopefully the process was to copy the Linux Tegra30 DT verbatim?
>>
>> No, the T20 one is far from verbatim neither. So I just did the
>> adjustments analogous by comparing the T20 and T30 Linux DTs.
>
> 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.
>
>> > That's
>> > far more likely to yield a correct DT than copying the Tegra20 DT to
>> > Tegra30 and then patching it until it works.
>>
>> I guess U-Boot has anyway about zero functionality dependency on that
>> part of the device trees.
>
> 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.
>
>> > If this DT doesn't exactly
>> > match the Linux kernel, this needs to be fixed.
>>
>> Well, then any Tegra device tree currently in U-Boot needs serious
>> fixing. I usually tend to at least not make the mess any worse.
>
> 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).
>
> 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'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.
>
>> > > diff --git a/arch/arm/mach-tegra/tegra30/Makefile b/arch/arm/mach-tegra/tegra30/Makefile
>> >
>> > > -obj-$(CONFIG_SPL_BUILD) += cpu.o
>> > > +ifdef CONFIG_SPL_BUILD
>> > > +obj-y        += cpu.o
>> >
>> >
>> > I don't think there's any need to edit the cpu.o line. While you can
>> > move it into the ifdef like that, I don't see a need.
>>
>> I can sure revert this then.
>>
>> > > diff --git a/arch/arm/mach-tegra/tegra30/display.c b/arch/arm/mach-tegra/tegra30/display.c
>> >
>> > I didn't review this file in detail; I'll leave that to Thierry since he
>> > knows the display HW.
>> >
>> > However, one question: Is this file a complete cut/paste of
>> > tegra20/display.c, or does it just replace some parts of it? Hopefully
>> > this patch doesn't simply duplicate the whole driver?
>>
>> Yes, for now this is  an exact one-to-one copy but I lack the detailed
>> knowledge about Tegra graphics to know whether this is much sane.

I have serious doubts about the wisdom of requiring a contributor to
completely re-architect the existing display system in U-Boot. It's a
big job. Perhaps we can settle for following along the same lines and
not making things worse?

That said, if Marcel has the time, I may as well pile on with a few
more comments :-)

>
> 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. 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.
>
> 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.
>
>> On first sight to me the current split between hardware agnostic (e.g.
>> driver/video/tegra.c) and hardware specific (e.g.
>> mach-tegra/<SoC>/display.c) seems rather arbitrary. Downstream [1] I
>> actually took a more radical approach and if that is rather accepted
>> I'm happy to rework this patch set in that direction as well. But then
>> actually completely merging tegra.c and display.c might even make more
>> sense. I'm open to suggestions really.
>>
>> [1] http://git.toradex.com/cgit/u-boot-toradex.git/commit/?h=2015.04-toradex-next&id=5a472ddd7a2a017747d6c05c65eba2cd3804c02f
>
> 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);

Please don't add function points to structures on an ad-hoc basis.
These should use driver model. There is a uclass for display port but
not for LCD panels or SOR. You could add a very simple one for a panel
if you like. Please take a look at tegra124's display driver for an
example.

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

Regards,
Simon