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

[Thierry Reding]

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.

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.

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

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

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20150825/ce477939/attachment.sig>