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

[Simon Glass]

Hi Thierry,

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:
>> +Nikita
>>
>> Hi Thierry,
>>
>> On 24 August 2015 at 04:12, Thierry Reding <treding at nvidia.com> wrote:
>> > On Fri, Aug 21, 2015 at 06:37:37PM -0600, Simon Glass wrote:
>> > [...]
>> >> 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?
>> >
>> > I didn't suggest re-architecting the display system in U-Boot. What I
>> > was suggesting was a way to architect Tegra-specific display driver code
>> > to make it reusable rather than duplicate display controller programming
>> > for each new generation, while the hardware has remained mostly the
>> > same.
>>
>> OK, I misunderstood.
>>
>> >
>> >> > 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.
>> >
>> > I don't think the driver model is a good fit here. Abstracting a display
>> > port isn't very useful in itself because users don't really care about
>> > the type of display, they only care about it being a display. So if you
>> > want to usefully abstract you'd do it at a higher level, such as display
>> > or screen. Then you have a generic object which users can use to put up
>> > a framebuffer onto a physical screen.
>>
>> I think you are referring to the lcd/video interface. If so, this is
>> already fairly well defined, but lcd and video should be merged, and a
>> uclass could be added. Nikita Kiryanov has done quite a bit of work on
>> the merging side.
>>
>> But I still think there is value in a low-level abstraction too.
>> Function pointers indicate that there is an interface that can be used
>> by multiple drivers, and that is what driver model is for. See
>> displayport.h for an attempt at this. We can of course consider
>> expanding the display port uclass to encompass panels in general. I
>> was reluctant to do that with a sample size of one. Here is the
>> current interface:
>>
>> /**
>>  * display_port_read_edid() - Read information from EDID
>>  *
>>  * @dev:        Device to read from
>>  * @buf:        Buffer to read into (should be EDID_SIZE bytes)
>>  * @buf_size:   Buffer size (should be EDID_SIZE)
>>  * @return number of bytes read, <=0 for error
>>  */
>> int display_port_read_edid(struct udevice *dev, u8 *buf, int buf_size);
>>
>> /**
>>  * display_port_enable() - Enable a display port device
>>  *
>>  * @dev:        Device to enable
>>  * @panel_bpp:  Number of bits per pixel for panel
>>  * @timing:     Display timings
>>  * @return 0 if OK, -ve on error
>>  */
>> int display_port_enable(struct udevice *dev, int panel_bpp,
>>                         const struct display_timing *timing);
>
> Both of these really aren't specific to DisplayPort. A DSI or HDMI input
> also wants to be enabled or have its EDID queried. Well, EDID may not be
> available on most DSI panels, so I think this particular abstraction
> should be slightly higher-level. What users are interested in isn't the
> EDID information, but the content therein. So I think a better way to
> return this type of information is by generating a list of modes (or a
> single one) given a display output device.

That sounds reasonable.

>
> And once you have that abstraction it becomes useless to abstract the
> various types of outputs, because DisplayPort, LVDS, HDMI, DSI, etc.
> will all behave the same.

OK so once we have another user we should rename this to something
like VIDEOPORT...

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

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

Regards,
Simon