Re: [PATCH v5 1/7] drm: sunxi: Add a basic DRM driver for Allwinner DE2

2 messages, 2 authors, 2016-11-17 · open the first message on its own page

Re: [PATCH v5 1/7] drm: sunxi: Add a basic DRM driver for Allwinner DE2

From: Maxime Ripard <hidden>
Date: 2016-11-16 21:33:06

On Tue, Nov 08, 2016 at 03:37:52PM +0100, Jean-Francois Moine wrote:

On Mon, 7 Nov 2016 23:37:41 +0100
Maxime Ripard [off-list ref] wrote:

quoted

Hi,

On Fri, Oct 28, 2016 at 07:34:20PM +0200, Jean-Francois Moine wrote:

quoted

On Fri, 28 Oct 2016 00:03:16 +0200
Maxime Ripard [off-list ref] wrote:

	[snip]

quoted

We've been calling them bus and mod.

I can understand "bus" (which is better than "apb"), but why "mod"?

Allwinner has been calling the clocks that are supposed to generate
the external signals (depending on where you were looking) module or
mod clocks (which is also why we have mod in the clock
compatibles). The module 1 clocks being used for the audio and the
module 0 for the rest (SPI, MMC, NAND, display, etc.)

I did not find any 'module' in the H3 documentation.
So, is it really a good name?

It's true that they use it less nowadays, but they still do,
ie. https://github.com/allwinner-zh/linux-3.4-sunxi/blob/master/drivers/clk/sunxi/clk-sun8iw7.c#L513

There is a 'mod' suffix, but it is used for the bus gates only, not for
the main clocks.

quoted

And we have to remain consistent anyway.

I don't see any consistency in the H3 DT:
- the bus gates are named "ahb" and apb"

We've been inconsistent in the past, hence why it would be great to
have bus instead.

- the (main) clocks are named "mmc", "usb0_phy" and "ir"
There is no "bus" nor "mod".

Look into the other devices, for other SoCs. Pointing out that we've
been bad in the past doesn't mean that we can't improve.

quoted

+
+- resets: phandle to the reset of the device
+
+- ports: phandle's to the LCD ports

Please use the OF graph.

These ports are references to the graph of nodes. See
	http://www.kernelhub.org/?msg=911825&p=2

In an OF-graph, your phandle to the LCD controller would be replaced
by an output endpoint.

This is the DE controller. There is no endpoint link at this level.

The display engine definitely has an endpoint: the TCON.

Not at all. The video chain is simply
	CRTC (TCON) -> connector (HDMI/LCD/DAC/..)
The DE is an ancillary device which handles the planes.

And what does it do exactly with those planes? It outputs it to the
TCON.

quoted

The Device Engine just handles the planes of the LCDs, but, indeed,
the LCDs must know about the DE and the DE must know about the LCDs.
There are 2 ways to realize this knowledge in the DT:
1) either the DE has one or two phandle's to the LCDs,
2) or the LCDs have a phandle to the DE.

I chose the 1st way, the DE ports pointing to the endpoint of the LCDs
which is part of the video link (OF-graph LCD <-> connector).
It would be possible to have phandles to the LCDs themselves, but this
asks for more code.

The second way is also possible, but it also complexifies a bit the
exchanges DE <-> LCD.

I'm still not sure how it would complexify anything, and why you can't
use the display graph to model the relation between the display engine
and the TCON (and why you want to use a generic property that refers
to the of-graph while it really isn't).

Complexification:
1- my solution:
  At startup time, the DE device is the DRM device.

How do you deal with SoCs with multiple display engines then?

It has to know the devices entering in the video chains.
  The CRTCs (LCD/TCON) are found by
	ports[i] -> parent
  The connectors are found by
	ports[i] -> endpoint -> remote_endpoint -> parent
2- with ports pointing to the LCDs:
  The CRTCs (LCD/TCON) are simply
	ports[i]
  The connectors are found by
	loop on all ports of ports[i]
		ports[i][j] -> endpoint -> remote_endpoint -> parent
3- with a phandle to the DE in the LCDs:

What do you mean with LCD? Panels? Why would panels have a phandle to
the display engine?

  The DE cannot be the DRM device because there is no information about
  the video devices in the DT. Then, the DRM devices are the LCDs.
  These LCDs must give their indices to the DE. So, the DE must implement
  some callback function to accept a LCD definition, and there must be
  a list of DEs in the driver to make the association DE <-> LCD[i]
  Some more problem may be raised if a user wants to have the same frame
  buffer on the 2 LCDs of a DE.

I have no idea what you're talking about, sorry.

Anyway, my solution is already used in the IMX Soc.
See 'display-subsystem' in arch/arm/boot/dts/imx6q.dtsi for an example.

Pointing out random example in the tree doesn't make a compelling
argument.

quoted

+void de2_disable_vblank(struct drm_device *drm, unsigned crtc)
+{
+	struct priv *priv = drm->dev_private;
+	struct lcd *lcd = priv->lcds[crtc];
+
+	tcon_write(lcd->mmio, gint0,
+			 tcon_read(lcd->mmio, gint0) &
+					~TCON_GINT0_TCON1_Vb_Int_En);
+}
+
+/* panel functions */

Panel functions? In the CRTC driver?

Yes, dumb panel.

What do you mean by that? Using a Parallel/RGB interface?

Sorry, I though this was a well-known name. The 'dump panel' was used
in the documentation of my previous ARM machine as the video frame sent
to the HDMI controller. 'video_frame' is OK for you?

If it's the frame sent to the encoder, then it would be the CRTC by
DRM's nomenclature.

The CRTC is a software entity. The frame buffer is a hardware entity.

Why are you about framebuffer now, this is nowhere in that
discussion. Any way, the framebuffer is also what is put in a plane,
so there's a name collision here, and you'll probably want to change
it.

Judging by the previous discussion, this should really be called
encoder if it encodes the frames on a bus format, or CRTC if it
composes the planes.

quoted

+static const struct {
+	char chan;
+	char layer;
+	char pipe;
+} plane2layer[DE2_N_PLANES] = {
+	[DE2_PRIMARY_PLANE] =	{0, 0, 0},
+	[DE2_CURSOR_PLANE] =	{1, 0, 1},
+	[DE2_VI_PLANE] =	{0, 1, 0},
+};

	[snip]

quoted

Comments?

This
	primary plane is channel 0 (VI), layer 0, pipe 0
	cursor plane is channel 1 (UI), layer 0, pipe 1
	overlay plane is channel 0 (VI), layer 1, pipe 0
or the full explanation:
    Constraints:
	The VI channels can do RGB or YUV, while UI channels can do RGB
	only.
	The LCD 0 has 1 VI channel and 4 UI channels, while
	LCD 1 has only 1 VI channel and 1 UI channel.
	The cursor must go to a channel bigger than the primary channel,
	otherwise it is not transparent.
    First try:
	Letting the primary plane (usually RGB) in the 2nd channel (UI),
	as this is done in the legacy driver, asks for the cursor to go
	to the next channel (UI), but this one does not exist in LCD1.
    Retained layout:
	So, we must use only 2 channels for the same behaviour on LCD0
	(H3) and LCD1 (A83T)
	The retained combination is:
		- primary plane in the first channel (VI),
		- cursor plane inthe 2nd channel (UI), and
		- overlay plane in the 1st channel (VI).

	Note that there could be 3 overlay planes (a channel has 4
	layers), but I am not sure that the A83T or the H3 could
	support 3 simultaneous video streams...

Do you know if the pipe works in the old display engine?

Especially about the two-steps composition that wouldn't allow you to
have alpha on all the planes?

If it is similar, I think hardcoding the pipe number is pretty bad,
because that would restrict the combination of planes and formats,
while some other might have worked.

From what I understood about the DE2, the pipes just define the priority
of the overlay channels (one pipe for one channel).
With the cursor constraint, there must be at least 2 channels in
order (primary, cursor). Then, with these 2 channels/pipes, there can be
6 so-called overlay planes (3 RGB/YUV and 3 RGB only).
Enabling the pipes 2 and 3 (LCD 0 only) would offer 8 more planes, but
RGB only. Then, it might be useful to have dynamic pipes.

That's very valuable (and definitely should go into a comment),
thanks!

I still believe that's it should be into a (simple at first)
atomic_check. That would be easier to extend and quite easy to
document and get simply by looking at the code.

Sorry for I don't understand what you mean.

You can check all the constraints you exposed above in atomic_check
instead of hardcoding it.

quoted

+static int __init de2_drm_init(void)
+{
+	int ret;
+
+/* uncomment to activate the drm traces at startup time */
+/*	drm_debug = DRM_UT_CORE | DRM_UT_DRIVER | DRM_UT_KMS |
+			DRM_UT_PRIME | DRM_UT_ATOMIC; */

That's useless.

Right, but it seems that some people don't know how to debug a DRM
driver. This is only a reminder.

quoted

+	DRM_DEBUG_DRIVER("\n");
+
+	ret = platform_driver_register(&de2_lcd_platform_driver);
+	if (ret < 0)
+		return ret;
+
+	ret = platform_driver_register(&de2_drm_platform_driver);
+	if (ret < 0)
+		platform_driver_unregister(&de2_lcd_platform_driver);
+
+	return ret;
+}

And that really shouldn't be done that way.

May you explain?

This goes against the whole idea of the device and driver
model. Drivers should only register themselves, device should be
created by buses (or by using some external components if the bus
can't: DT, ACPI, etc.). If there's a match, you get probed.

A driver that creates its own device just to probe itself violates
that.

In this function (module init), there is no driver yet.
The module contains 2 drivers: the DE (planes) and the LCD (CRTC),
and there is no macro to handle such modules.

Ah, yes, my bad. I thought you were registering a device and a
driver. Still this is a very unusual pattern. Why do you need to split
the two? Can't you just merge them?

The DE and the LCDs are different devices on different drivers.
A DE must be only one device because it has to handle concurent
accesses from its 2 LCDs. Then 2 drivers.

If it's different drivers, why are they in the same module?

But only one module. Why? Because there cannot be double direction
calls from one module to an other one, and, in our case, for example,
- the DRM (DE) device must call vblank functions which are handled in
  the CRTC (TCON) device, and
- the CRTC device must call DE initialization functions at startup time.

I'm sorry, but that doesn't make any sense. The crtc device should
take care of the CRTC functions. Your DRM CRTC and encoders can
definitely be shared across different devices, you can have a look at
how we're doing it for sun4i.

We basically have 3 drivers to create most of the display pipeline:
  - One for the DRM device
  - One for the display engine
  - One for the TCON

Once they have all loaded and registered in the component framework,
their bind callback is called, and it's when the DRM entities are
created using exported functions to manipulate what needs to be setup.

It's definitely doable, it just takes some effort.

quoted

+int de2_plane_init(struct drm_device *drm, struct lcd *lcd)
+{
+	int ret, possible_crtcs = 1 << lcd->crtc_idx;
+
+	ret = de2_one_plane_init(drm, &lcd->planes[DE2_PRIMARY_PLANE],
+				DRM_PLANE_TYPE_PRIMARY, possible_crtcs,
+				ui_formats, ARRAY_SIZE(ui_formats));
+	if (ret >= 0)
+		ret = de2_one_plane_init(drm, &lcd->planes[DE2_CURSOR_PLANE],
+				DRM_PLANE_TYPE_CURSOR, possible_crtcs,
+				ui_formats, ARRAY_SIZE(ui_formats));

Nothing looks really special about that cursor plane. Any reasion not
to make it an overlay?

As explained above (channel/layer/pipe plane definitions), the cursor
cannot go in a channel lower or equal to the one of the primary plane.
Then, it must be known and, so, have an explicit plane.

If you were to make it a plane, you could use atomic_check to check
this and make sure this doesn't happen. And you would gain a generic
plane that can be used for other purposes if needed.

The function drm_crtc_init_with_planes() offers a cursor plane for free.
On the other side, having 6 overlay planes is more than the SoCs can
support.

It's not really for free, it costs you a generic plane that could
definitely be used for something else and cannot anymore because
they've been hardcoded to a cursor.

And having a camera, the VPU or even an application directly output
directly into one of these planes seems a much better use of a generic
plane than a cursor.

Looking at the harder case (A83T), there may be 8 planes on 2 channels.
Using a primary plane and the cursor,
	8 planes - primary plane - cursor plane = 6 planes
6 planes are available.
If I count correctly, in your example:
	one camera + one VPU + one application = 3 planes
3 planes are used.
So, 3 planes are still available.

On the other side, removing the cursor would just let one more plane.
Do we really need this one? In other words, I'd be pleased to know how
you run 7 applications doing video overlay.

You can use those planes to do composition too, with each application
having one or more plane. Android uses that.

There's no argument to have a cursor plane, really. Even regular
graphic stack like X can use a regular overlay to have its cursor on
it. It doesn't *remove* anything, it just allows to use the plane for
what it was supposed to be used for.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20161116/98fa4f0f/attachment.sig>

Re: [PATCH v5 1/7] drm: sunxi: Add a basic DRM driver for Allwinner DE2

From: Jean-Francois Moine <hidden>
Date: 2016-11-17 14:57:07

On Wed, 16 Nov 2016 22:33:06 +0100
Maxime Ripard [off-list ref] wrote:

quoted

The Device Engine just handles the planes of the LCDs, but, indeed,
the LCDs must know about the DE and the DE must know about the LCDs.
There are 2 ways to realize this knowledge in the DT:
1) either the DE has one or two phandle's to the LCDs,
2) or the LCDs have a phandle to the DE.

I chose the 1st way, the DE ports pointing to the endpoint of the LCDs
which is part of the video link (OF-graph LCD <-> connector).
It would be possible to have phandles to the LCDs themselves, but this
asks for more code.

The second way is also possible, but it also complexifies a bit the
exchanges DE <-> LCD.

I'm still not sure how it would complexify anything, and why you can't
use the display graph to model the relation between the display engine
and the TCON (and why you want to use a generic property that refers
to the of-graph while it really isn't).

Complexification:
1- my solution:
  At startup time, the DE device is the DRM device.

How do you deal with SoCs with multiple display engines then?

In the H3, A83T and A64, there is only one DE.
If many DEs in a SoC, there may be either one DRM device per DE or one
DRM device for the whole system. In this last case, the (global) DE
would have many resources (many I/O memory maps / IRQs..) and the
physical DE of each endpoint would be indicated by the position of its
phandle in the 'ports' array (DT documentation).

quoted

It has to know the devices entering in the video chains.
  The CRTCs (LCD/TCON) are found by
	ports[i] -> parent
  The connectors are found by
	ports[i] -> endpoint -> remote_endpoint -> parent
2- with ports pointing to the LCDs:
  The CRTCs (LCD/TCON) are simply
	ports[i]
  The connectors are found by
	loop on all ports of ports[i]
		ports[i][j] -> endpoint -> remote_endpoint -> parent
3- with a phandle to the DE in the LCDs:

What do you mean with LCD? Panels? Why would panels have a phandle to
the display engine?

LCD is the same as CRTC. I don't think people will connect old CRT's to
their new ARM boards. LCD's may be panels, modern TV sets, or any
digital display. The word LCD seems clearer to me in this context, even
if there may a DAC as an ancoder.

quoted

  The DE cannot be the DRM device because there is no information about
  the video devices in the DT. Then, the DRM devices are the LCDs.
  These LCDs must give their indices to the DE. So, the DE must implement
  some callback function to accept a LCD definition, and there must be
  a list of DEs in the driver to make the association DE <-> LCD[i]
  Some more problem may be raised if a user wants to have the same frame
  buffer on the 2 LCDs of a DE.

I have no idea what you're talking about, sorry.

Here is the DT (I am using back 'CRTC'):

	de: display-controller at xxx {
		...
	};
	crtc0: crt-controller at xxx{
		...
		display-controller = <&de>;
		ports {
			...		/* to the crtc0 connectors */
		}
	};
	crtc1: crt-controller at xxx{
		...
		display-controller = <&de>;
		ports {
			...		/* to the crtc1 connectors */
		};
	};

There are 2 DRM devices: one on crtc0, the other one on crtc1.
The DE device is isolated. But, to treat the planes, it must receive
information about the CRTCs. How?

quoted

Anyway, my solution is already used in the IMX Soc.
See 'display-subsystem' in arch/arm/boot/dts/imx6q.dtsi for an example.

Pointing out random example in the tree doesn't make a compelling
argument.

This is not a random example. There was a thread about the 'ports'
phandle in the DT definition of the IMX video subsystem, and what kind
of OF function should be used (see one of my previous mails). In the DRI
list, nobody objected about the phandle by itself.

quoted

Panel functions? In the CRTC driver?

Yes, dumb panel.

What do you mean by that? Using a Parallel/RGB interface?

Sorry, I though this was a well-known name. The 'dump panel' was used
in the documentation of my previous ARM machine as the video frame sent
to the HDMI controller. 'video_frame' is OK for you?

If it's the frame sent to the encoder, then it would be the CRTC by
DRM's nomenclature.

The CRTC is a software entity. The frame buffer is a hardware entity.

Why are you about framebuffer now, this is nowhere in that
discussion. Any way, the framebuffer is also what is put in a plane,
so there's a name collision here, and you'll probably want to change
it.

Judging by the previous discussion, this should really be called
encoder if it encodes the frames on a bus format, or CRTC if it
composes the planes.

I think that we will end in agreeing on the words. We just need some
time!
Here is how I understand the Allwinner's DE2:

- the TCON is the piece of hardware which gets some memory area and
  sends it to a bus according to some configuation (screen resolution,
  timings...). The bus data are encoded and transmitted to the connector.
  At the end, the display device receives frames. So, going back to the
  TCON, the memory are is the frame buffer.

- this frame buffer is virtual, empty, 'dumb', it is a dumb panel!
  It is filled by planes. This job is done by a specific image
  processor, the one contained in the DE2.

- the DE2 processor gets the plane source images from the SoC memory.
  It adjusts the images according to many configuration parameters and
  includes the result into the frame buffer.

So:
	LCD = CRTC = frame buffer = dumb panel = TCON

- LCD = hardware piece of a display device (terminal side) which renders
  the colored pixels in a digital way. By extension, the hardware part
  (computer side) of a display engine which handles the definitions of
  a digital or analog physical display device. By extension also,
  the software structure (driver side) which defines a physical screen.

- CRTC = DRM software entity which handles the definitions of a screen,
  but not just a CRT.

- frame buffer = piece of memory which contains the images which are
  sent to a screen.

- dumb panel = abstract entity which defines the characteristics of a
  physical screen.

You may note that, in the DE2 scheme, the TCON and LCD are not in the
same (software) device while they are part of the same DRM software
entity, the CTRC.

quoted

If it is similar, I think hardcoding the pipe number is pretty bad,
because that would restrict the combination of planes and formats,
while some other might have worked.

From what I understood about the DE2, the pipes just define the priority
of the overlay channels (one pipe for one channel).
With the cursor constraint, there must be at least 2 channels in
order (primary, cursor). Then, with these 2 channels/pipes, there can be
6 so-called overlay planes (3 RGB/YUV and 3 RGB only).
Enabling the pipes 2 and 3 (LCD 0 only) would offer 8 more planes, but
RGB only. Then, it might be useful to have dynamic pipes.

That's very valuable (and definitely should go into a comment),
thanks!

I still believe that's it should be into a (simple at first)
atomic_check. That would be easier to extend and quite easy to
document and get simply by looking at the code.

Sorry for I don't understand what you mean.

You can check all the constraints you exposed above in atomic_check
instead of hardcoding it.

Sorry, but I don't like to run useless code for pure static definition.

quoted

The DE and the LCDs are different devices on different drivers.
A DE must be only one device because it has to handle concurent
accesses from its 2 LCDs. Then 2 drivers.

If it's different drivers, why are they in the same module?

quoted

But only one module. Why? Because there cannot be double direction
calls from one module to an other one, and, in our case, for example,
- the DRM (DE) device must call vblank functions which are handled in
  the CRTC (TCON) device, and
- the CRTC device must call DE initialization functions at startup time.

I'm sorry, but that doesn't make any sense. The crtc device should
take care of the CRTC functions. Your DRM CRTC and encoders can
definitely be shared across different devices, you can have a look at
how we're doing it for sun4i.

We basically have 3 drivers to create most of the display pipeline:
  - One for the DRM device
  - One for the display engine
  - One for the TCON

Your DRM device is useless. It is simpler to have the DRM device as the
display engine.
Also, maybe, you have not the constraint the DE being shared between
2 CRTCs.

Once they have all loaded and registered in the component framework,
their bind callback is called, and it's when the DRM entities are
created using exported functions to manipulate what needs to be setup.

It's definitely doable, it just takes some effort.

It seems you did not look at what I have coded...

quoted

On the other side, removing the cursor would just let one more plane.
Do we really need this one? In other words, I'd be pleased to know how
you run 7 applications doing video overlay.

You can use those planes to do composition too, with each application
having one or more plane. Android uses that.

There's no argument to have a cursor plane, really. Even regular
graphic stack like X can use a regular overlay to have its cursor on
it. It doesn't *remove* anything, it just allows to use the plane for
what it was supposed to be used for.

I'd be glad to know how you can have a hardware cursor without defining
it in drm_crtc_init_with_planes().

-- 
Ken ar c'henta?	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help