[Patch 0/4] drm/omap: Add virtual-planes support

[Patch 0/4] drm/omap: Add virtual-planes support

[Benoit Parrot]

This patch series adds virtual-plane support to omapdrm driver
to allow the use of display wider than 2048 pixels.

The DT bindings are also cleaned up to remove duplication when
properties are common to all implementations.

Benoit Parrot (4):
  dt-bindings: display/ti: Move common dispc bindings to omap-dss.txt
  dt-bindings: display/ti: Add plane binding to dispc node
  drm/omap: Add virtual plane DT parsing support
  drm/omap: Add virtual plane support to omap_plane

 .../devicetree/bindings/display/ti/ti,dra7-dss.txt |   5 -
 .../devicetree/bindings/display/ti/ti,omap-dss.txt |  70 +++++++++++
 .../bindings/display/ti/ti,omap2-dss.txt           |   4 -
 .../bindings/display/ti/ti,omap3-dss.txt           |   4 -
 .../bindings/display/ti/ti,omap4-dss.txt           |   4 -
 .../bindings/display/ti/ti,omap5-dss.txt           |   4 -
 drivers/gpu/drm/omapdrm/dss/dispc.c                | 110 ++++++++++++++++
 drivers/gpu/drm/omapdrm/dss/omapdss.h              |  11 ++
 drivers/gpu/drm/omapdrm/omap_drv.c                 |  18 ++-
 drivers/gpu/drm/omapdrm/omap_fb.c                  |  66 ++++++----
 drivers/gpu/drm/omapdrm/omap_fb.h                  |   4 +-
 drivers/gpu/drm/omapdrm/omap_plane.c               | 139 +++++++++++++++------
 drivers/gpu/drm/omapdrm/omap_plane.h               |   3 +-
 13 files changed, 353 insertions(+), 89 deletions(-)

-- 
2.9.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[Patch 1/4] dt-bindings: display/ti: Move common dispc bindings to omap-dss.txt

[Benoit Parrot]

Add common DISPC bindings into the top level bindings file.
Move common bindings here instead of having multiple copies of
the same information in all of the variant specific files.

Signed-off-by: Benoit Parrot <bparrot@ti.com>
---
 Documentation/devicetree/bindings/display/ti/ti,dra7-dss.txt  | 5 -----
 Documentation/devicetree/bindings/display/ti/ti,omap-dss.txt  | 7 +++++++
 Documentation/devicetree/bindings/display/ti/ti,omap2-dss.txt | 4 ----
 Documentation/devicetree/bindings/display/ti/ti,omap3-dss.txt | 4 ----
 Documentation/devicetree/bindings/display/ti/ti,omap4-dss.txt | 4 ----
 Documentation/devicetree/bindings/display/ti/ti,omap5-dss.txt | 4 ----
 6 files changed, 7 insertions(+), 21 deletions(-)

diff --git a/Documentation/devicetree/bindings/display/ti/ti,dra7-dss.txt b/Documentation/devicetree/bindings/display/ti/ti,dra7-dss.txt
index 91279f1060fe..c30f9ec189ed 100644
--- a/Documentation/devicetree/bindings/display/ti/ti,dra7-dss.txt
+++ b/Documentation/devicetree/bindings/display/ti/ti,dra7-dss.txt
@@ -47,11 +47,6 @@ Required properties:
 - clocks: handle to fclk
 - clock-names: "fck"
 
-Optional properties:
-- max-memory-bandwidth: Input memory (from main memory to dispc) bandwidth limit
-			in bytes per second
-
-
 HDMI
 ----
 
diff --git a/Documentation/devicetree/bindings/display/ti/ti,omap-dss.txt b/Documentation/devicetree/bindings/display/ti/ti,omap-dss.txt
index e1ef29569338..249e588d7865 100644
--- a/Documentation/devicetree/bindings/display/ti/ti,omap-dss.txt
+++ b/Documentation/devicetree/bindings/display/ti/ti,omap-dss.txt
@@ -21,6 +21,13 @@ a RGB pixel stream to encoders.
 The encoder modules encode the received RGB pixel stream to a video output like
 HDMI, MIPI DPI, etc.
 
+DISPC
+-----
+
+Optional properties:
+- max-memory-bandwidth: Input memory (from main memory to dispc) bandwidth limit
+			in bytes per second
+
 Video Ports
 -----------
 
diff --git a/Documentation/devicetree/bindings/display/ti/ti,omap2-dss.txt b/Documentation/devicetree/bindings/display/ti/ti,omap2-dss.txt
index ee867c4d1152..afcd5a86c6a4 100644
--- a/Documentation/devicetree/bindings/display/ti/ti,omap2-dss.txt
+++ b/Documentation/devicetree/bindings/display/ti/ti,omap2-dss.txt
@@ -28,10 +28,6 @@ Required properties:
 - ti,hwmods: "dss_dispc"
 - interrupts: the DISPC interrupt
 
-Optional properties:
-- max-memory-bandwidth: Input memory (from main memory to dispc) bandwidth limit
-			in bytes per second
-
 
 RFBI
 ----
diff --git a/Documentation/devicetree/bindings/display/ti/ti,omap3-dss.txt b/Documentation/devicetree/bindings/display/ti/ti,omap3-dss.txt
index cd02516a40b6..dc66e1447c31 100644
--- a/Documentation/devicetree/bindings/display/ti/ti,omap3-dss.txt
+++ b/Documentation/devicetree/bindings/display/ti/ti,omap3-dss.txt
@@ -37,10 +37,6 @@ Required properties:
 - clocks: handle to fclk
 - clock-names: "fck"
 
-Optional properties:
-- max-memory-bandwidth: Input memory (from main memory to dispc) bandwidth limit
-			in bytes per second
-
 
 RFBI
 ----
diff --git a/Documentation/devicetree/bindings/display/ti/ti,omap4-dss.txt b/Documentation/devicetree/bindings/display/ti/ti,omap4-dss.txt
index 0f85f6b3a5a8..bc624db8888d 100644
--- a/Documentation/devicetree/bindings/display/ti/ti,omap4-dss.txt
+++ b/Documentation/devicetree/bindings/display/ti/ti,omap4-dss.txt
@@ -36,10 +36,6 @@ Required properties:
 - clocks: handle to fclk
 - clock-names: "fck"
 
-Optional properties:
-- max-memory-bandwidth: Input memory (from main memory to dispc) bandwidth limit
-			in bytes per second
-
 
 RFBI
 ----
diff --git a/Documentation/devicetree/bindings/display/ti/ti,omap5-dss.txt b/Documentation/devicetree/bindings/display/ti/ti,omap5-dss.txt
index 20861218649f..118a486c47bb 100644
--- a/Documentation/devicetree/bindings/display/ti/ti,omap5-dss.txt
+++ b/Documentation/devicetree/bindings/display/ti/ti,omap5-dss.txt
@@ -36,10 +36,6 @@ Required properties:
 - clocks: handle to fclk
 - clock-names: "fck"
 
-Optional properties:
-- max-memory-bandwidth: Input memory (from main memory to dispc) bandwidth limit
-			in bytes per second
-
 
 RFBI
 ----
-- 
2.9.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[Patch 2/4] dt-bindings: display/ti: Add plane binding to dispc node

[Benoit Parrot]

Add 'plane' child node to generic DISPC node as an optional
property.

Signed-off-by: Benoit Parrot <bparrot@ti.com>
---
 .../devicetree/bindings/display/ti/ti,omap-dss.txt | 63 ++++++++++++++++++++++
 1 file changed, 63 insertions(+)

diff --git a/Documentation/devicetree/bindings/display/ti/ti,omap-dss.txt b/Documentation/devicetree/bindings/display/ti/ti,omap-dss.txt
index 249e588d7865..cb101525b805 100644
--- a/Documentation/devicetree/bindings/display/ti/ti,omap-dss.txt
+++ b/Documentation/devicetree/bindings/display/ti/ti,omap-dss.txt
@@ -27,6 +27,34 @@ DISPC
 Optional properties:
 - max-memory-bandwidth: Input memory (from main memory to dispc) bandwidth limit
 			in bytes per second
+- plane: Child node(s) which defines which logical plane are available to
+	the system. If at least one plane child node is defined then
+	only planes defined by these nodes will be available to the system.
+	Plane nodes must be sequential starting with reg = <0> as DT parsing
+	will stop on the first missing numbered node.
+	This means if plane #1 is defined but plane #0 is not then it will
+	be as if none of the plane nodes were defined.
+
+	Each plane node contains the following properties:
+	Required properties:
+	- reg:       Used to number the logical plane
+	- hw-planes: One or two HW plane number(s).
+		     When 2 numbers are present this indicates a virtual plane
+		     composed of two physical planes intended to be used
+		     when the display is larger then the capacity of a
+		     single plane i.e. wider than 2048 pixels.
+		     The first number in the pair will dictate the capabilities
+		     of the virtual plane. This means that for proper
+		     operation the virtual plane should be composed of HW
+		     planes of the same capabilities.
+		     If GFX plane is used in a virtual plane it should be
+		     specified first, otherwise unexpected behavior would
+		     be encountered.
+	Optional property:
+	- hw-crtcs:  One or more HW crtc number(s).
+		     Describe the list of CRTCs on which this plane is
+		     available. If this node is not present then the
+		     plane will be available on all available CRTCs.
 
 Video Ports
 -----------
@@ -216,3 +244,38 @@ OMAP HDMI --(HDMI)--> TPD12S015 --(HDMI)--> HDMI Connector
 		};
 	};
 };
+
+A short example on how to define a virtual plane configuration
+to enable wide display support.
+Here we define:
+- plane#0 to be the HW plane #0 (i.e. GFX plane)
+	  only available on crtc #0
+- plane#1 to be a virtual wide plane composed of HW plane #1 and #2
+	  (i.e. VID1 & VID2) available on crtc #0 & #1
+- plane#2 to be the HW plane #3 (i.e. VID3 plane)
+	  only available on crtc #0
+
+&dss {
+        dispc@58001000 {
+                #address-cells = <1>;
+                #size-cells = <0>;
+
+                plane@0 {
+                        reg = <0>;
+                        hw-planes = <0>;
+                        hw-crtcs = <0>;
+                };
+
+                plane@1 {
+                        reg = <1>;
+                        hw-planes = <1 2>;
+                        hw-crtcs = <0 1>;
+                };
+
+                plane@2 {
+                        reg = <2>;
+                        hw-planes = <3>;
+                        hw-crtcs = <0>;
+                };
+        };
+};
-- 
2.9.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[Patch 3/4] drm/omap: Add virtual plane DT parsing support

[Benoit Parrot]

Virtual planes are used to extend display size capability for display
larger than 2048 pixels by splitting the frame buffer equally between
two physical planes.

Here we are adding DT support to parse 'plane' child nodes which
describe how logical planes are mapped to physical plane(s) and
which crtc they are available on.

Signed-off-by: Benoit Parrot <bparrot@ti.com>
---
 drivers/gpu/drm/omapdrm/dss/dispc.c   | 110 ++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/omapdrm/dss/omapdss.h |  11 ++++
 2 files changed, 121 insertions(+)

diff --git a/drivers/gpu/drm/omapdrm/dss/dispc.c b/drivers/gpu/drm/omapdrm/dss/dispc.c
index 624dee22f46b..559b70d9762d 100644
--- a/drivers/gpu/drm/omapdrm/dss/dispc.c
+++ b/drivers/gpu/drm/omapdrm/dss/dispc.c
@@ -4334,6 +4334,115 @@ static u32 dispc_get_memory_bandwidth_limit(void)
 	return limit;
 }
 
+static struct device_node *dispc_of_get_plane_by_id(struct device_node *node,
+						    u32 id)
+{
+	struct device_node *plane;
+
+	for_each_child_of_node(node, plane) {
+		u32 plane_id = 0;
+
+		if (of_node_cmp(plane->name, "plane") != 0)
+			continue;
+		of_property_read_u32(plane, "reg", &plane_id);
+		if (id == plane_id)
+			break;
+	}
+
+	return plane;
+}
+
+static int dispc_parse_dt_plane_data(struct dispc_plane_mappings *plane)
+{
+	struct platform_device *pdev = dispc.pdev;
+	struct device_node *np = pdev->dev.of_node;
+	struct device_node *ep;
+	struct property *prop;
+	const __be32 *cur;
+	u32 v;
+	u32 num_ovls = dispc_get_num_ovls();
+	unsigned long int hw_plane_mask = (1 << num_ovls) - 1;
+	u32 num_planes;
+	int i, index;
+
+	if (!np)
+		return 0;
+
+	for (i = 0; i < num_ovls; i++) {
+		ep = dispc_of_get_plane_by_id(np, i);
+		if (!ep)
+			break;
+		if (!of_property_read_bool(ep, "hw-planes")) {
+			dev_err(&pdev->dev,
+				"malformed plane node: hw-planes missing.\n");
+			return -EINVAL;
+		}
+
+		index = 0;
+		of_property_for_each_u32(ep, "hw-planes", prop, cur, v) {
+			if (v >= num_ovls) {
+				dev_err(&pdev->dev,
+					"hw-planes property: '%d' out-of-range.\n",
+					v);
+				return -EINVAL;
+			}
+			if (!(hw_plane_mask & BIT_MASK(v))) {
+				dev_err(&pdev->dev,
+					"hw-planes property: '%d' used more than once.\n",
+					v);
+				return -EINVAL;
+			}
+			clear_bit(v, &hw_plane_mask);
+
+			if (index == 0) {
+				plane->plane[i].main_id = v;
+			} else if (index == 1) {
+				plane->plane[i].aux_id = v;
+				plane->plane[i].is_virtual = true;
+			} else if (index > 1) {
+				dev_err(&pdev->dev,
+					"hw-planes property: more than 2 values specified.\n");
+				return -EINVAL;
+			}
+			index++;
+		}
+
+		of_property_for_each_u32(ep, "hw-crtcs", prop, cur, v) {
+			if (v >= num_ovls) {
+				dev_err(&pdev->dev,
+					"hw-crtcs property: '%d' out-of-range.\n",
+					v);
+				return -EINVAL;
+			}
+			plane->plane[i].crtc_mask |= 1 << v;
+		}
+	}
+
+	num_planes = i;
+
+	if (num_planes) {
+		dev_dbg(&pdev->dev, "Plane definitions found from DT:");
+		for (i = 0; i < num_planes; i++) {
+			if (plane->plane[i].is_virtual) {
+				dev_dbg(&pdev->dev,
+					"plane%d: virtual hw-planes: %d, %d crtc_mask: 0x%04x",
+					i, plane->plane[i].main_id,
+					plane->plane[i].aux_id,
+					plane->plane[i].crtc_mask);
+			} else {
+				dev_dbg(&pdev->dev,
+					"plane%d: hw-planes: %d crtc_mask: 0x%04x",
+					i, plane->plane[i].main_id,
+					plane->plane[i].crtc_mask);
+			}
+		}
+	}
+
+	plane->num_planes = num_planes;
+
+	return 0;
+}
+
 /*
  * Workaround for errata i734 in DSS dispc
  *  - LCD1 Gamma Correction Is Not Working When GFX Pipe Is Disabled
@@ -4525,6 +4634,7 @@ static const struct dispc_ops dispc_ops = {
 	.ovl_enable = dispc_ovl_enable,
 	.ovl_setup = dispc_ovl_setup,
 	.ovl_get_color_modes = dispc_ovl_get_color_modes,
+	.get_plane_mapping = dispc_parse_dt_plane_data,
 };
 
 /* DISPC HW IP initialisation */
diff --git a/drivers/gpu/drm/omapdrm/dss/omapdss.h b/drivers/gpu/drm/omapdrm/dss/omapdss.h
index f8f83e826a56..b8c9b30bf5ed 100644
--- a/drivers/gpu/drm/omapdrm/dss/omapdss.h
+++ b/drivers/gpu/drm/omapdrm/dss/omapdss.h
@@ -637,6 +637,16 @@ void omapdss_set_is_initialized(bool set);
 struct device_node *dss_of_port_get_parent_device(struct device_node *port);
 u32 dss_of_port_get_port_number(struct device_node *port);
 
+struct dispc_plane_mappings {
+	struct {
+		u32 main_id;
+		u32 aux_id;
+		u32 crtc_mask;
+		bool	is_virtual;
+	} plane[4];
+	u32 num_planes;
+};
+
 struct dss_mgr_ops {
 	int (*connect)(enum omap_channel channel,
 		struct omap_dss_device *dst);
@@ -719,6 +729,7 @@ struct dispc_ops {
 			enum omap_channel channel);
 
 	const u32 *(*ovl_get_color_modes)(enum omap_plane_id plane);
+	int (*get_plane_mapping)(struct dispc_plane_mappings *plane);
 };
 
 void dispc_set_ops(const struct dispc_ops *o);
-- 
2.9.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[Patch 4/4] drm/omap: Add virtual plane support to omap_plane

[Benoit Parrot]

Add virtual plane support by adding an array to contain
all of the actual plane_id a "omap_plane" correspond to.

When at least one 'plane' child node is present in DT then
omap_plane_init will only used the plane described in DT.
Some of these nodes may be a virtual plane if they are defined
as two physical planes.
Planes can also be associated with various crtcs independently.
Therefore we can restrict the use of virtual plane to specific
CRTC/video port if need be, if crtc_mask is not specified then
the plane will be available to all available crtcs.
Physical plane which are not described will essentially be hidden
from the driver.

If no 'plane' child node exist then the existing plane
allocation will take place.

Signed-off-by: Benoit Parrot <bparrot@ti.com>
---
 drivers/gpu/drm/omapdrm/omap_drv.c   |  18 +++--
 drivers/gpu/drm/omapdrm/omap_fb.c    |  66 +++++++++++------
 drivers/gpu/drm/omapdrm/omap_fb.h    |   4 +-
 drivers/gpu/drm/omapdrm/omap_plane.c | 139 +++++++++++++++++++++++++----------
 drivers/gpu/drm/omapdrm/omap_plane.h |   3 +-
 5 files changed, 162 insertions(+), 68 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c
index dd68b2556f5b..73796364a592 100644
--- a/drivers/gpu/drm/omapdrm/omap_drv.c
+++ b/drivers/gpu/drm/omapdrm/omap_drv.c
@@ -188,10 +188,9 @@ static int omap_connect_dssdevs(void)
 	return r;
 }
 
-static int omap_modeset_init_properties(struct drm_device *dev)
+static int omap_modeset_init_properties(struct drm_device *dev, u32 num_planes)
 {
 	struct omap_drm_private *priv = dev->dev_private;
-	unsigned int num_planes = priv->dispc_ops->get_num_ovls();
 
 	priv->zorder_prop = drm_property_create_range(dev, 0, "zorder", 0,
 						      num_planes - 1);
@@ -210,10 +209,19 @@ static int omap_modeset_init(struct drm_device *dev)
 	int num_crtcs, crtc_idx, plane_idx;
 	int ret;
 	u32 plane_crtc_mask;
+	struct dispc_plane_mappings plane_mappings = {0};
 
 	drm_mode_config_init(dev);
 
-	ret = omap_modeset_init_properties(dev);
+	ret = priv->dispc_ops->get_plane_mapping(&plane_mappings);
+	if (ret < 0)
+		return ret;
+
+	/* use plane mappings info */
+	if (plane_mappings.num_planes)
+		num_ovls = plane_mappings.num_planes;
+
+	ret = omap_modeset_init_properties(dev, num_ovls);
 	if (ret < 0)
 		return ret;
 
@@ -266,7 +274,7 @@ static int omap_modeset_init(struct drm_device *dev)
 			return -ENOMEM;
 
 		plane = omap_plane_init(dev, plane_idx, DRM_PLANE_TYPE_PRIMARY,
-					plane_crtc_mask);
+					plane_crtc_mask, &plane_mappings);
 		if (IS_ERR(plane))
 			return PTR_ERR(plane);
 
@@ -296,7 +304,7 @@ static int omap_modeset_init(struct drm_device *dev)
 			return -EINVAL;
 
 		plane = omap_plane_init(dev, plane_idx, DRM_PLANE_TYPE_OVERLAY,
-			plane_crtc_mask);
+					plane_crtc_mask, &plane_mappings);
 		if (IS_ERR(plane))
 			return PTR_ERR(plane);
 
diff --git a/drivers/gpu/drm/omapdrm/omap_fb.c b/drivers/gpu/drm/omapdrm/omap_fb.c
index b2539a90e1a4..80b29b7f5696 100644
--- a/drivers/gpu/drm/omapdrm/omap_fb.c
+++ b/drivers/gpu/drm/omapdrm/omap_fb.c
@@ -153,25 +153,27 @@ static uint32_t drm_rotation_to_tiler(unsigned int drm_rot)
 /* update ovl info for scanout, handles cases of multi-planar fb's, etc.
  */
 void omap_framebuffer_update_scanout(struct drm_framebuffer *fb,
-		struct drm_plane_state *state, struct omap_overlay_info *info)
+		struct drm_plane_state *state,
+		struct omap_overlay_info *main_info,
+		struct omap_overlay_info *aux_info)
 {
 	struct omap_framebuffer *omap_fb = to_omap_framebuffer(fb);
 	const struct drm_format_info *format = omap_fb->format;
 	struct plane *plane = &omap_fb->planes[0];
 	uint32_t x, y, orient = 0;
 
-	info->fourcc = fb->format->format;
+	main_info->fourcc = fb->format->format;
 
-	info->pos_x      = state->crtc_x;
-	info->pos_y      = state->crtc_y;
-	info->out_width  = state->crtc_w;
-	info->out_height = state->crtc_h;
-	info->width      = state->src_w >> 16;
-	info->height     = state->src_h >> 16;
+	main_info->pos_x      = state->crtc_x;
+	main_info->pos_y      = state->crtc_y;
+	main_info->out_width  = state->crtc_w;
+	main_info->out_height = state->crtc_h;
+	main_info->width      = state->src_w >> 16;
+	main_info->height     = state->src_h >> 16;
 
 	/* DSS driver wants the w & h in rotated orientation */
 	if (drm_rotation_90_or_270(state->rotation))
-		swap(info->width, info->height);
+		swap(main_info->width, main_info->height);
 
 	x = state->src_x >> 16;
 	y = state->src_y >> 16;
@@ -202,11 +204,12 @@ void omap_framebuffer_update_scanout(struct drm_framebuffer *fb,
 
 		/* Note: x and y are in TILER units, not pixels */
 		omap_gem_rotated_dma_addr(plane->bo, orient, x, y,
-					  &info->paddr);
-		info->rotation_type = OMAP_DSS_ROT_TILER;
-		info->rotation = state->rotation ?: DRM_MODE_ROTATE_0;
+					  &main_info->paddr);
+		main_info->rotation_type = OMAP_DSS_ROT_TILER;
+		main_info->rotation = state->rotation ?: DRM_MODE_ROTATE_0;
 		/* Note: stride in TILER units, not pixels */
-		info->screen_width  = omap_gem_tiled_stride(plane->bo, orient);
+		main_info->screen_width  =
+				omap_gem_tiled_stride(plane->bo, orient);
 	} else {
 		switch (state->rotation & DRM_MODE_ROTATE_MASK) {
 		case 0:
@@ -221,27 +224,46 @@ void omap_framebuffer_update_scanout(struct drm_framebuffer *fb,
 			break;
 		}
 
-		info->paddr         = get_linear_addr(plane, format, 0, x, y);
-		info->rotation_type = OMAP_DSS_ROT_NONE;
-		info->rotation      = DRM_MODE_ROTATE_0;
-		info->screen_width  = plane->pitch;
+		main_info->paddr = get_linear_addr(plane, format, 0, x, y);
+		main_info->rotation_type = OMAP_DSS_ROT_NONE;
+		main_info->rotation = DRM_MODE_ROTATE_0;
+		main_info->screen_width = plane->pitch;
 	}
 
 	/* convert to pixels: */
-	info->screen_width /= format->cpp[0];
+	main_info->screen_width /= format->cpp[0];
 
 	if (fb->format->format == DRM_FORMAT_NV12) {
 		plane = &omap_fb->planes[1];
 
-		if (info->rotation_type == OMAP_DSS_ROT_TILER) {
+		if (main_info->rotation_type == OMAP_DSS_ROT_TILER) {
 			WARN_ON(!(omap_gem_flags(plane->bo) & OMAP_BO_TILED));
 			omap_gem_rotated_dma_addr(plane->bo, orient, x/2, y/2,
-						  &info->p_uv_addr);
+						  &main_info->p_uv_addr);
 		} else {
-			info->p_uv_addr = get_linear_addr(plane, format, 1, x, y);
+			main_info->p_uv_addr =
+				get_linear_addr(plane, format, 1, x, y);
 		}
 	} else {
-		info->p_uv_addr = 0;
+		main_info->p_uv_addr = 0;
+	}
+
+	if (aux_info) {
+		main_info->width /= 2;
+		main_info->out_width /= 2;
+
+		*aux_info = *main_info;
+
+		aux_info->pos_x = main_info->pos_x + main_info->out_width;
+
+		aux_info->paddr =
+				get_linear_addr(&omap_fb->planes[0], format, 0,
+						x + main_info->width, y);
+		if (fb->format->format == DRM_FORMAT_NV12) {
+			aux_info->p_uv_addr =
+				get_linear_addr(&omap_fb->planes[1], format, 1,
+						x + main_info->width, y);
+		}
 	}
 }
 
diff --git a/drivers/gpu/drm/omapdrm/omap_fb.h b/drivers/gpu/drm/omapdrm/omap_fb.h
index 94ad5f9e4404..f68e81353288 100644
--- a/drivers/gpu/drm/omapdrm/omap_fb.h
+++ b/drivers/gpu/drm/omapdrm/omap_fb.h
@@ -37,7 +37,9 @@ struct drm_framebuffer *omap_framebuffer_init(struct drm_device *dev,
 int omap_framebuffer_pin(struct drm_framebuffer *fb);
 void omap_framebuffer_unpin(struct drm_framebuffer *fb);
 void omap_framebuffer_update_scanout(struct drm_framebuffer *fb,
-		struct drm_plane_state *state, struct omap_overlay_info *info);
+		struct drm_plane_state *state,
+		struct omap_overlay_info *main_info,
+		struct omap_overlay_info *aux_info);
 struct drm_connector *omap_framebuffer_get_next_connector(
 		struct drm_framebuffer *fb, struct drm_connector *from);
 bool omap_framebuffer_supports_rotation(struct drm_framebuffer *fb);
diff --git a/drivers/gpu/drm/omapdrm/omap_plane.c b/drivers/gpu/drm/omapdrm/omap_plane.c
index 7d789d1551a1..e3e6623c405d 100644
--- a/drivers/gpu/drm/omapdrm/omap_plane.c
+++ b/drivers/gpu/drm/omapdrm/omap_plane.c
@@ -30,10 +30,14 @@
 
 struct omap_plane {
 	struct drm_plane base;
-	enum omap_plane_id id;
+	enum omap_plane_id main_id;
+	enum omap_plane_id aux_id;
 	const char *name;
+	bool virtual_plane;
 };
 
+static const char *plane_id_to_name[];
+
 static int omap_plane_prepare_fb(struct drm_plane *plane,
 				 struct drm_plane_state *new_state)
 {
@@ -56,38 +60,70 @@ static void omap_plane_atomic_update(struct drm_plane *plane,
 	struct omap_drm_private *priv = plane->dev->dev_private;
 	struct omap_plane *omap_plane = to_omap_plane(plane);
 	struct drm_plane_state *state = plane->state;
-	struct omap_overlay_info info;
+	struct omap_overlay_info main_info, aux_info;
 	int ret;
+	bool dual_plane = omap_plane->virtual_plane;
 
 	DBG("%s, crtc=%p fb=%p", omap_plane->name, state->crtc, state->fb);
 
-	memset(&info, 0, sizeof(info));
-	info.rotation_type = OMAP_DSS_ROT_NONE;
-	info.rotation = DRM_MODE_ROTATE_0;
-	info.global_alpha = 0xff;
-	info.zorder = state->zpos;
+	memset(&main_info, 0, sizeof(main_info));
+	main_info.rotation_type = OMAP_DSS_ROT_NONE;
+	main_info.rotation = DRM_MODE_ROTATE_0;
+	main_info.global_alpha = 0xff;
+	main_info.zorder = state->zpos;
 
-	/* update scanout: */
-	omap_framebuffer_update_scanout(state->fb, state, &info);
+	aux_info = main_info;
 
-	DBG("%dx%d -> %dx%d (%d)", info.width, info.height,
-			info.out_width, info.out_height,
-			info.screen_width);
-	DBG("%d,%d %pad %pad", info.pos_x, info.pos_y,
-			&info.paddr, &info.p_uv_addr);
+	/* update scanout: */
+	omap_framebuffer_update_scanout(state->fb, state, &main_info,
+					dual_plane ? &aux_info : NULL);
+
+	DBG("%s: %dx%d -> %dx%d (%d)",
+	    plane_id_to_name[omap_plane->main_id],
+	    main_info.width, main_info.height,
+	    main_info.out_width, main_info.out_height,
+	    main_info.screen_width);
+	DBG("%d,%d %pad %pad", main_info.pos_x, main_info.pos_y,
+	    &main_info.paddr, &main_info.p_uv_addr);
+
+	if (dual_plane) {
+		aux_info.zorder = main_info.zorder + 1; // XXX
+
+		DBG("%s: %dx%d -> %dx%d (%d)",
+		    plane_id_to_name[omap_plane->aux_id],
+		    aux_info.width, aux_info.height,
+		    aux_info.out_width, aux_info.out_height,
+		    aux_info.screen_width);
+		DBG("%d,%d %pad %pad", aux_info.pos_x, aux_info.pos_y,
+		    &aux_info.paddr, &aux_info.p_uv_addr);
+	}
 
-	/* and finally, update omapdss: */
-	ret = priv->dispc_ops->ovl_setup(omap_plane->id, &info,
+	ret = priv->dispc_ops->ovl_setup(omap_plane->main_id, &main_info,
 			      omap_crtc_timings(state->crtc), false,
 			      omap_crtc_channel(state->crtc));
 	if (ret) {
-		dev_err(plane->dev->dev, "Failed to setup plane %s\n",
+		dev_err(plane->dev->dev, "Failed to setup plane1 %s\n",
 			omap_plane->name);
-		priv->dispc_ops->ovl_enable(omap_plane->id, false);
+		priv->dispc_ops->ovl_enable(omap_plane->main_id, false);
 		return;
 	}
 
-	priv->dispc_ops->ovl_enable(omap_plane->id, true);
+	priv->dispc_ops->ovl_enable(omap_plane->main_id, true);
+
+	if (dual_plane) {
+		ret = priv->dispc_ops->ovl_setup(omap_plane->aux_id, &aux_info,
+				      omap_crtc_timings(state->crtc), false,
+				      omap_crtc_channel(state->crtc));
+		if (ret) {
+			dev_err(plane->dev->dev, "Failed to setup plane2 %s\n",
+				omap_plane->name);
+			priv->dispc_ops->ovl_enable(omap_plane->aux_id, false);
+			priv->dispc_ops->ovl_enable(omap_plane->main_id, false);
+			return;
+		}
+
+		priv->dispc_ops->ovl_enable(omap_plane->aux_id, true);
+	}
 }
 
 static void omap_plane_atomic_disable(struct drm_plane *plane,
@@ -95,12 +131,15 @@ static void omap_plane_atomic_disable(struct drm_plane *plane,
 {
 	struct omap_drm_private *priv = plane->dev->dev_private;
 	struct omap_plane *omap_plane = to_omap_plane(plane);
+	bool dual_plane = omap_plane->virtual_plane;
 
 	plane->state->rotation = DRM_MODE_ROTATE_0;
 	plane->state->zpos = plane->type == DRM_PLANE_TYPE_PRIMARY
-			   ? 0 : omap_plane->id;
+			   ? 0 : omap_plane->main_id;
 
-	priv->dispc_ops->ovl_enable(omap_plane->id, false);
+	priv->dispc_ops->ovl_enable(omap_plane->main_id, false);
+	if (dual_plane)
+		priv->dispc_ops->ovl_enable(omap_plane->aux_id, false);
 }
 
 static int omap_plane_atomic_check(struct drm_plane *plane,
@@ -195,7 +234,7 @@ static void omap_plane_reset(struct drm_plane *plane)
 	 * plane.
 	 */
 	plane->state->zpos = plane->type == DRM_PLANE_TYPE_PRIMARY
-			   ? 0 : omap_plane->id;
+			   ? 0 : omap_plane->main_id;
 }
 
 static int omap_plane_atomic_set_property(struct drm_plane *plane,
@@ -246,43 +285,65 @@ static const char *plane_id_to_name[] = {
 	[OMAP_DSS_VIDEO3] = "vid3",
 };
 
-static const enum omap_plane_id plane_idx_to_id[] = {
-	OMAP_DSS_GFX,
-	OMAP_DSS_VIDEO1,
-	OMAP_DSS_VIDEO2,
-	OMAP_DSS_VIDEO3,
+static const char * const virtual_plane_id_to_name[] = {
+	[OMAP_DSS_GFX] = "virt-gfx",
+	[OMAP_DSS_VIDEO1] = "virt-vid1",
+	[OMAP_DSS_VIDEO2] = "virt-vid2",
+	[OMAP_DSS_VIDEO3] = "virt-vid3",
 };
 
-/* initialize plane */
 struct drm_plane *omap_plane_init(struct drm_device *dev,
 		int idx, enum drm_plane_type type,
-		u32 possible_crtcs)
+		u32 possible_crtcs,
+		struct dispc_plane_mappings *plane_mappings)
 {
 	struct omap_drm_private *priv = dev->dev_private;
 	unsigned int num_planes = priv->dispc_ops->get_num_ovls();
 	struct drm_plane *plane;
 	struct omap_plane *omap_plane;
-	enum omap_plane_id id;
 	int ret;
 	u32 nformats;
 	const u32 *formats;
 
-	if (WARN_ON(idx >= ARRAY_SIZE(plane_idx_to_id)))
-		return ERR_PTR(-EINVAL);
+	if (plane_mappings->num_planes)
+		num_planes = plane_mappings->num_planes;
 
-	id = plane_idx_to_id[idx];
-
-	DBG("%s: type=%d", plane_id_to_name[id], type);
+	if (WARN_ON(idx >= num_planes))
+		return ERR_PTR(-EINVAL);
 
 	omap_plane = kzalloc(sizeof(*omap_plane), GFP_KERNEL);
 	if (!omap_plane)
 		return ERR_PTR(-ENOMEM);
 
-	formats = priv->dispc_ops->ovl_get_color_modes(id);
+	if (plane_mappings->num_planes) {
+		/* Use plane data from DT */
+		omap_plane->main_id = plane_mappings->plane[idx].main_id;
+		if (plane_mappings->plane[idx].is_virtual) {
+			omap_plane->name =
+				virtual_plane_id_to_name[omap_plane->main_id];
+			omap_plane->aux_id = plane_mappings->plane[idx].aux_id;
+			omap_plane->virtual_plane = true;
+		} else {
+			omap_plane->name =
+				plane_id_to_name[omap_plane->main_id];
+		}
+		if (plane_mappings->plane[idx].crtc_mask)
+			possible_crtcs = plane_mappings->plane[idx].crtc_mask;
+	} else {
+		/* Use legacy plane allocation */
+		omap_plane->main_id = idx;
+		omap_plane->name = plane_id_to_name[idx];
+	}
+
+	DBG("%s: type=%d", omap_plane->name, type);
+	DBG("	omap_plane->main_id: %d", omap_plane->main_id);
+	if (omap_plane->virtual_plane)
+		DBG("	omap_plane->aux_id: %d", omap_plane->aux_id);
+	DBG("	crtc_mask: 0x%04x", possible_crtcs);
+
+	formats = priv->dispc_ops->ovl_get_color_modes(omap_plane->main_id);
 	for (nformats = 0; formats[nformats]; ++nformats)
 		;
-	omap_plane->id = id;
-	omap_plane->name = plane_id_to_name[id];
 
 	plane = &omap_plane->base;
 
@@ -301,7 +362,7 @@ struct drm_plane *omap_plane_init(struct drm_device *dev,
 
 error:
 	dev_err(dev->dev, "%s(): could not create plane: %s\n",
-		__func__, plane_id_to_name[id]);
+		__func__, omap_plane->name);
 
 	kfree(omap_plane);
 	return NULL;
diff --git a/drivers/gpu/drm/omapdrm/omap_plane.h b/drivers/gpu/drm/omapdrm/omap_plane.h
index dc5e82ad061d..ad09471d4601 100644
--- a/drivers/gpu/drm/omapdrm/omap_plane.h
+++ b/drivers/gpu/drm/omapdrm/omap_plane.h
@@ -30,7 +30,8 @@ struct drm_plane;
 
 struct drm_plane *omap_plane_init(struct drm_device *dev,
 		int idx, enum drm_plane_type type,
-		u32 possible_crtcs);
+		u32 possible_crtcs,
+		struct dispc_plane_mappings *plane_mappings);
 void omap_plane_install_properties(struct drm_plane *plane,
 		struct drm_mode_object *obj);
 
-- 
2.9.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Patch 2/4] dt-bindings: display/ti: Add plane binding to dispc node

[Rob Herring]

On Fri, Mar 2, 2018 at 7:48 AM, Benoit Parrot <bparrot@ti.com> wrote:
> Add 'plane' child node to generic DISPC node as an optional
> property.

Why? What problem are you solving?

>
> Signed-off-by: Benoit Parrot <bparrot@ti.com>
> ---
>  .../devicetree/bindings/display/ti/ti,omap-dss.txt | 63 ++++++++++++++++++++++
>  1 file changed, 63 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/display/ti/ti,omap-dss.txt b/Documentation/devicetree/bindings/display/ti/ti,omap-dss.txt
> index 249e588d7865..cb101525b805 100644
> --- a/Documentation/devicetree/bindings/display/ti/ti,omap-dss.txt
> +++ b/Documentation/devicetree/bindings/display/ti/ti,omap-dss.txt
> @@ -27,6 +27,34 @@ DISPC
>  Optional properties:
>  - max-memory-bandwidth: Input memory (from main memory to dispc) bandwidth limit
>                         in bytes per second
> +- plane: Child node(s) which defines which logical plane are available to

This is the "Optional properties" section and plane is not a property.

> +       the system. If at least one plane child node is defined then
> +       only planes defined by these nodes will be available to the system.
> +       Plane nodes must be sequential starting with reg = <0> as DT parsing
> +       will stop on the first missing numbered node.
> +       This means if plane #1 is defined but plane #0 is not then it will
> +       be as if none of the plane nodes were defined.
> +
> +       Each plane node contains the following properties:
> +       Required properties:
> +       - reg:       Used to number the logical plane

Is logical plane a h/w concept?

> +       - hw-planes: One or two HW plane number(s).
> +                    When 2 numbers are present this indicates a virtual plane
> +                    composed of two physical planes intended to be used
> +                    when the display is larger then the capacity of a
> +                    single plane i.e. wider than 2048 pixels.
> +                    The first number in the pair will dictate the capabilities
> +                    of the virtual plane. This means that for proper
> +                    operation the virtual plane should be composed of HW
> +                    planes of the same capabilities.
> +                    If GFX plane is used in a virtual plane it should be
> +                    specified first, otherwise unexpected behavior would
> +                    be encountered.
> +       Optional property:
> +       - hw-crtcs:  One or more HW crtc number(s).
> +                    Describe the list of CRTCs on which this plane is
> +                    available. If this node is not present then the
> +                    plane will be available on all available CRTCs.

Let's not copy archaic terms from DRM into bindings.

Really, I'm skeptical that any of this belongs in DT. For example,
can't you figure out you need 2 physical planes whenever your
panel/timing width is greater than 2048?

Rob
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Patch 1/4] dt-bindings: display/ti: Move common dispc bindings to omap-dss.txt

[Rob Herring]

On Fri, Mar 02, 2018 at 07:48:01AM -0600, Benoit Parrot wrote:
> Add common DISPC bindings into the top level bindings file.
> Move common bindings here instead of having multiple copies of
> the same information in all of the variant specific files.
> 
> Signed-off-by: Benoit Parrot <bparrot@ti.com>
> ---
>  Documentation/devicetree/bindings/display/ti/ti,dra7-dss.txt  | 5 -----
>  Documentation/devicetree/bindings/display/ti/ti,omap-dss.txt  | 7 +++++++
>  Documentation/devicetree/bindings/display/ti/ti,omap2-dss.txt | 4 ----
>  Documentation/devicetree/bindings/display/ti/ti,omap3-dss.txt | 4 ----
>  Documentation/devicetree/bindings/display/ti/ti,omap4-dss.txt | 4 ----
>  Documentation/devicetree/bindings/display/ti/ti,omap5-dss.txt | 4 ----
>  6 files changed, 7 insertions(+), 21 deletions(-)

Reviewed-by: Rob Herring <robh@kernel.org>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Patch 2/4] dt-bindings: display/ti: Add plane binding to dispc node

[Benoit Parrot]

Rob Herring <robh+dt@kernel.org> wrote on Fri [2018-Mar-02 13:19:13 -0600]:
> On Fri, Mar 2, 2018 at 7:48 AM, Benoit Parrot <bparrot@ti.com> wrote:
> > Add 'plane' child node to generic DISPC node as an optional
> > property.
> 
> Why? What problem are you solving?

Ah yes, I guess on its own it does not mean much...

How about:

Currently all available display pipelines (i.e. plane) and output
port resources are exposed to user-space.
In some cases it is needed to be able restrict which resources are
actually visible from user-space. Also in cases where a display wider
than 2048 pixels is to be supported more than one video pipeline is
needed. In this case the 2nd hardware pipeline needed is not visible
to user space applications.

These video pipeline definitions must be statically defined so that
the number of visible pipelines does not change from the user-space
perspective.

In order to allow this we are adding an optional sub-node to the generic
DISPC node. 

> 
> >
> > Signed-off-by: Benoit Parrot <bparrot@ti.com>
> > ---
> >  .../devicetree/bindings/display/ti/ti,omap-dss.txt | 63 ++++++++++++++++++++++
> >  1 file changed, 63 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/display/ti/ti,omap-dss.txt b/Documentation/devicetree/bindings/display/ti/ti,omap-dss.txt
> > index 249e588d7865..cb101525b805 100644
> > --- a/Documentation/devicetree/bindings/display/ti/ti,omap-dss.txt
> > +++ b/Documentation/devicetree/bindings/display/ti/ti,omap-dss.txt
> > @@ -27,6 +27,34 @@ DISPC
> >  Optional properties:
> >  - max-memory-bandwidth: Input memory (from main memory to dispc) bandwidth limit
> >                         in bytes per second
> > +- plane: Child node(s) which defines which logical plane are available to
> 
> This is the "Optional properties" section and plane is not a property.

Right, I'll correct that to an optional sub-node which has required and
optional properties.

> 
> > +       the system. If at least one plane child node is defined then
> > +       only planes defined by these nodes will be available to the system.
> > +       Plane nodes must be sequential starting with reg = <0> as DT parsing
> > +       will stop on the first missing numbered node.
> > +       This means if plane #1 is defined but plane #0 is not then it will
> > +       be as if none of the plane nodes were defined.
> > +
> > +       Each plane node contains the following properties:
> > +       Required properties:
> > +       - reg:       Used to number the logical plane
> 
> Is logical plane a h/w concept?

It does represent a hardware resource.

> 
> > +       - hw-planes: One or two HW plane number(s).
> > +                    When 2 numbers are present this indicates a virtual plane
> > +                    composed of two physical planes intended to be used
> > +                    when the display is larger then the capacity of a
> > +                    single plane i.e. wider than 2048 pixels.
> > +                    The first number in the pair will dictate the capabilities
> > +                    of the virtual plane. This means that for proper
> > +                    operation the virtual plane should be composed of HW
> > +                    planes of the same capabilities.
> > +                    If GFX plane is used in a virtual plane it should be
> > +                    specified first, otherwise unexpected behavior would
> > +                    be encountered.
> > +       Optional property:
> > +       - hw-crtcs:  One or more HW crtc number(s).
> > +                    Describe the list of CRTCs on which this plane is
> > +                    available. If this node is not present then the
> > +                    plane will be available on all available CRTCs.
> 
> Let's not copy archaic terms from DRM into bindings.

Ok, I can rename them to use the TRM terminology instead.
I chose DRM term because they are well known.
So we'll have

video-pipeline instead of hw-planes
video-output instead of hw-crtcs

Any comments on the sub-node name itself?
Or can we keep 'plane'?

> 
> Really, I'm skeptical that any of this belongs in DT. For example,
> can't you figure out you need 2 physical planes whenever your
> panel/timing width is greater than 2048?

As stated in the description I added above, we cannot have resources
exposed to user-space which can "disappear" dynamically.
Doing so would break user-space applications which rely on these
resources.

Benoit

> 
> Rob
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Patch 3/4] drm/omap: Add virtual plane DT parsing support

[Tomi Valkeinen]

Hi Benoit,

On 02/03/18 15:48, Benoit Parrot wrote:
> Virtual planes are used to extend display size capability for display
> larger than 2048 pixels by splitting the frame buffer equally between
> two physical planes.
> 
> Here we are adding DT support to parse 'plane' child nodes which
> describe how logical planes are mapped to physical plane(s) and
> which crtc they are available on.
> 
> Signed-off-by: Benoit Parrot <bparrot@ti.com>
> ---
>  drivers/gpu/drm/omapdrm/dss/dispc.c   | 110 ++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/omapdrm/dss/omapdss.h |  11 ++++
>  2 files changed, 121 insertions(+)
> 
> diff --git a/drivers/gpu/drm/omapdrm/dss/dispc.c b/drivers/gpu/drm/omapdrm/dss/dispc.c
> index 624dee22f46b..559b70d9762d 100644
> --- a/drivers/gpu/drm/omapdrm/dss/dispc.c
> +++ b/drivers/gpu/drm/omapdrm/dss/dispc.c
> @@ -4334,6 +4334,115 @@ static u32 dispc_get_memory_bandwidth_limit(void)
>  	return limit;
>  }
>  
> +static struct device_node *dispc_of_get_plane_by_id(struct device_node *node,
> +						    u32 id)
> +{
> +	struct device_node *plane;
> +
> +	for_each_child_of_node(node, plane) {
> +		u32 plane_id = 0;
> +
> +		if (of_node_cmp(plane->name, "plane") != 0)
> +			continue;
> +		of_property_read_u32(plane, "reg", &plane_id);
> +		if (id == plane_id)
> +			break;

I think the flow is more readable, if here you "return plane", and at
the end of the func "return NULL".

> +	}
> +
> +	return plane;
> +}
> +
> +static int dispc_parse_dt_plane_data(struct dispc_plane_mappings *plane)

You could rename the parameter to "map", for example. Using 'plane'
there is quite confusing.

> +{
> +	struct platform_device *pdev = dispc.pdev;
> +	struct device_node *np = pdev->dev.of_node;
> +	struct device_node *ep;
> +	struct property *prop;
> +	const __be32 *cur;
> +	u32 v;
> +	u32 num_ovls = dispc_get_num_ovls();
> +	unsigned long int hw_plane_mask = (1 << num_ovls) - 1;

u32?

> +	u32 num_planes;
> +	int i, index;
> +
> +	if (!np)
> +		return 0;
> +
> +	for (i = 0; i < num_ovls; i++) {
> +		ep = dispc_of_get_plane_by_id(np, i);
> +		if (!ep)
> +			break;
> +		if (!of_property_read_bool(ep, "hw-planes")) {
> +			dev_err(&pdev->dev,
> +				"malformed plane node: hw-planes missing.\n");
> +			return -EINVAL;
> +		}
> +
> +		index = 0;
> +		of_property_for_each_u32(ep, "hw-planes", prop, cur, v) {
> +			if (v >= num_ovls) {
> +				dev_err(&pdev->dev,
> +					"hw-planes property: '%d' out-of-range.\n",
> +					v);
> +				return -EINVAL;
> +			}
> +			if (!(hw_plane_mask & BIT_MASK(v))) {
> +				dev_err(&pdev->dev,
> +					"hw-planes property: '%d' used more than once.\n",
> +					v);
> +				return -EINVAL;
> +			}
> +			clear_bit(v, &hw_plane_mask);

Why do you use hw_plane_mask this way? It feels inverse to me, usually
one sets a bit when something is there. And e.g. crtc_mask here is used
the other way.

> +
> +			if (index == 0) {
> +				plane->plane[i].main_id = v;
> +			} else if (index == 1) {
> +				plane->plane[i].aux_id = v;
> +				plane->plane[i].is_virtual = true;
> +			} else if (index > 1) {
> +				dev_err(&pdev->dev,
> +					"hw-planes property: more than 2 values specified.\n");
> +				return -EINVAL;
> +			}
> +			index++;
> +		}
> +
> +		of_property_for_each_u32(ep, "hw-crtcs", prop, cur, v) {
> +			if (v >= num_ovls) {

This should check against num_ovl_mgrs.

> +				dev_err(&pdev->dev,
> +					"hw-crtcs property: '%d' out-of-range.\n",
> +					v);
> +				return -EINVAL;
> +			}
> +			plane->plane[i].crtc_mask |= 1 << v;
> +		}
> +	}
> +
> +	num_planes = i;
> +
> +	if (num_planes) {
> +		dev_dbg(&pdev->dev, "Plane definitions found from DT:");
> +		for (i = 0; i < num_planes; i++) {
> +			if (plane->plane[i].is_virtual) {
> +				dev_dbg(&pdev->dev,
> +					"plane%d: virtual hw-planes: %d, %d crtc_mask: 0x%04x",
> +					i, plane->plane[i].main_id,
> +					plane->plane[i].aux_id,
> +					plane->plane[i].crtc_mask);
> +			} else {
> +				dev_dbg(&pdev->dev,
> +					"plane%d: hw-planes: %d crtc_mask: 0x%04x",
> +					i, plane->plane[i].main_id,
> +					plane->plane[i].crtc_mask);
> +			}
> +		}
> +	}
> +
> +	plane->num_planes = num_planes;
> +
> +	return 0;
> +}
> +
>  /*
>   * Workaround for errata i734 in DSS dispc
>   *  - LCD1 Gamma Correction Is Not Working When GFX Pipe Is Disabled
> @@ -4525,6 +4634,7 @@ static const struct dispc_ops dispc_ops = {
>  	.ovl_enable = dispc_ovl_enable,
>  	.ovl_setup = dispc_ovl_setup,
>  	.ovl_get_color_modes = dispc_ovl_get_color_modes,
> +	.get_plane_mapping = dispc_parse_dt_plane_data,
>  };
>  
>  /* DISPC HW IP initialisation */
> diff --git a/drivers/gpu/drm/omapdrm/dss/omapdss.h b/drivers/gpu/drm/omapdrm/dss/omapdss.h
> index f8f83e826a56..b8c9b30bf5ed 100644
> --- a/drivers/gpu/drm/omapdrm/dss/omapdss.h
> +++ b/drivers/gpu/drm/omapdrm/dss/omapdss.h
> @@ -637,6 +637,16 @@ void omapdss_set_is_initialized(bool set);
>  struct device_node *dss_of_port_get_parent_device(struct device_node *port);
>  u32 dss_of_port_get_port_number(struct device_node *port);
>  
> +struct dispc_plane_mappings {
> +	struct {
> +		u32 main_id;
> +		u32 aux_id;
> +		u32 crtc_mask;
> +		bool	is_virtual;

is_virtual has a tab whereas the others don't.

 Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Patch 2/4] dt-bindings: display/ti: Add plane binding to dispc node

[Tomi Valkeinen]

On 09/03/18 20:27, Benoit Parrot wrote:

>> Is logical plane a h/w concept?
> 
> It does represent a hardware resource.

Logical plane is not a hw concept, it just describes a group of one or
two HW planes. Then again, in the context of 2k+ displays, two HW planes
must always be used together, so that way it could be considered a
single HW resource.

>> Really, I'm skeptical that any of this belongs in DT. For example,
>> can't you figure out you need 2 physical planes whenever your
>> panel/timing width is greater than 2048?
> 
> As stated in the description I added above, we cannot have resources
> exposed to user-space which can "disappear" dynamically.
> Doing so would break user-space applications which rely on these
> resources.

The question is, if not in DT, then where? I agree that this is not
exactly describing the HW. But it can't be done dynamically either (or
at least we have not figured out a way). And it must be user configurable.

Module parameters are an option, but it would be somewhat difficult to
give all this information there. And also, if your board has a 2k+
display, you must have these configurations given to the driver, it's
not optional.

And while it's perhaps stretching the definitions a bit, I guess one
could argue that this describes the HW in a way: it describes how the HW
resources must be used if you have a display of 2k+ width, and is not as
such related to Linux or DRM.

 Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Patch 4/4] drm/omap: Add virtual plane support to omap_plane

[Tomi Valkeinen]

Hi Benoit,

On 02/03/18 15:48, Benoit Parrot wrote:
> Add virtual plane support by adding an array to contain
> all of the actual plane_id a "omap_plane" correspond to.

"plane_ids", "an", "corresponds"

> When at least one 'plane' child node is present in DT then
> omap_plane_init will only used the plane described in DT.

"use"

> Some of these nodes may be a virtual plane if they are defined
> as two physical planes.
> Planes can also be associated with various crtcs independently.
> Therefore we can restrict the use of virtual plane to specific
> CRTC/video port if need be, if crtc_mask is not specified then
> the plane will be available to all available crtcs.
> Physical plane which are not described will essentially be hidden

"planes"

> from the driver.
> 
> If no 'plane' child node exist then the existing plane

"nodes"

> allocation will take place.

Maybe "normal plane allocation"?

> 
> Signed-off-by: Benoit Parrot <bparrot@ti.com>
> ---
>  drivers/gpu/drm/omapdrm/omap_drv.c   |  18 +++--
>  drivers/gpu/drm/omapdrm/omap_fb.c    |  66 +++++++++++------
>  drivers/gpu/drm/omapdrm/omap_fb.h    |   4 +-
>  drivers/gpu/drm/omapdrm/omap_plane.c | 139 +++++++++++++++++++++++++----------
>  drivers/gpu/drm/omapdrm/omap_plane.h |   3 +-
>  5 files changed, 162 insertions(+), 68 deletions(-)
> 
> diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c
> index dd68b2556f5b..73796364a592 100644
> --- a/drivers/gpu/drm/omapdrm/omap_drv.c
> +++ b/drivers/gpu/drm/omapdrm/omap_drv.c
> @@ -188,10 +188,9 @@ static int omap_connect_dssdevs(void)
>  	return r;
>  }
>  
> -static int omap_modeset_init_properties(struct drm_device *dev)
> +static int omap_modeset_init_properties(struct drm_device *dev, u32 num_planes)
>  {
>  	struct omap_drm_private *priv = dev->dev_private;
> -	unsigned int num_planes = priv->dispc_ops->get_num_ovls();
>  
>  	priv->zorder_prop = drm_property_create_range(dev, 0, "zorder", 0,
>  						      num_planes - 1);
> @@ -210,10 +209,19 @@ static int omap_modeset_init(struct drm_device *dev)
>  	int num_crtcs, crtc_idx, plane_idx;
>  	int ret;
>  	u32 plane_crtc_mask;
> +	struct dispc_plane_mappings plane_mappings = {0};
>  
>  	drm_mode_config_init(dev);
>  
> -	ret = omap_modeset_init_properties(dev);
> +	ret = priv->dispc_ops->get_plane_mapping(&plane_mappings);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* use plane mappings info */
> +	if (plane_mappings.num_planes)
> +		num_ovls = plane_mappings.num_planes;
> +
> +	ret = omap_modeset_init_properties(dev, num_ovls);
>  	if (ret < 0)
>  		return ret;
>  
> @@ -266,7 +274,7 @@ static int omap_modeset_init(struct drm_device *dev)
>  			return -ENOMEM;
>  
>  		plane = omap_plane_init(dev, plane_idx, DRM_PLANE_TYPE_PRIMARY,
> -					plane_crtc_mask);
> +					plane_crtc_mask, &plane_mappings);
>  		if (IS_ERR(plane))
>  			return PTR_ERR(plane);
>  
> @@ -296,7 +304,7 @@ static int omap_modeset_init(struct drm_device *dev)
>  			return -EINVAL;
>  
>  		plane = omap_plane_init(dev, plane_idx, DRM_PLANE_TYPE_OVERLAY,
> -			plane_crtc_mask);
> +					plane_crtc_mask, &plane_mappings);
>  		if (IS_ERR(plane))
>  			return PTR_ERR(plane);
>  
> diff --git a/drivers/gpu/drm/omapdrm/omap_fb.c b/drivers/gpu/drm/omapdrm/omap_fb.c
> index b2539a90e1a4..80b29b7f5696 100644
> --- a/drivers/gpu/drm/omapdrm/omap_fb.c
> +++ b/drivers/gpu/drm/omapdrm/omap_fb.c
> @@ -153,25 +153,27 @@ static uint32_t drm_rotation_to_tiler(unsigned int drm_rot)
>  /* update ovl info for scanout, handles cases of multi-planar fb's, etc.
>   */
>  void omap_framebuffer_update_scanout(struct drm_framebuffer *fb,
> -		struct drm_plane_state *state, struct omap_overlay_info *info)
> +		struct drm_plane_state *state,
> +		struct omap_overlay_info *main_info,
> +		struct omap_overlay_info *aux_info)
>  {
>  	struct omap_framebuffer *omap_fb = to_omap_framebuffer(fb);
>  	const struct drm_format_info *format = omap_fb->format;
>  	struct plane *plane = &omap_fb->planes[0];
>  	uint32_t x, y, orient = 0;
>  
> -	info->fourcc = fb->format->format;
> +	main_info->fourcc = fb->format->format;
>  
> -	info->pos_x      = state->crtc_x;
> -	info->pos_y      = state->crtc_y;
> -	info->out_width  = state->crtc_w;
> -	info->out_height = state->crtc_h;
> -	info->width      = state->src_w >> 16;
> -	info->height     = state->src_h >> 16;
> +	main_info->pos_x      = state->crtc_x;
> +	main_info->pos_y      = state->crtc_y;
> +	main_info->out_width  = state->crtc_w;
> +	main_info->out_height = state->crtc_h;
> +	main_info->width      = state->src_w >> 16;
> +	main_info->height     = state->src_h >> 16;
>  
>  	/* DSS driver wants the w & h in rotated orientation */
>  	if (drm_rotation_90_or_270(state->rotation))
> -		swap(info->width, info->height);
> +		swap(main_info->width, main_info->height);
>  
>  	x = state->src_x >> 16;
>  	y = state->src_y >> 16;
> @@ -202,11 +204,12 @@ void omap_framebuffer_update_scanout(struct drm_framebuffer *fb,
>  
>  		/* Note: x and y are in TILER units, not pixels */
>  		omap_gem_rotated_dma_addr(plane->bo, orient, x, y,
> -					  &info->paddr);
> -		info->rotation_type = OMAP_DSS_ROT_TILER;
> -		info->rotation = state->rotation ?: DRM_MODE_ROTATE_0;
> +					  &main_info->paddr);
> +		main_info->rotation_type = OMAP_DSS_ROT_TILER;
> +		main_info->rotation = state->rotation ?: DRM_MODE_ROTATE_0;
>  		/* Note: stride in TILER units, not pixels */
> -		info->screen_width  = omap_gem_tiled_stride(plane->bo, orient);
> +		main_info->screen_width  =
> +				omap_gem_tiled_stride(plane->bo, orient);
>  	} else {
>  		switch (state->rotation & DRM_MODE_ROTATE_MASK) {
>  		case 0:
> @@ -221,27 +224,46 @@ void omap_framebuffer_update_scanout(struct drm_framebuffer *fb,
>  			break;
>  		}
>  
> -		info->paddr         = get_linear_addr(plane, format, 0, x, y);
> -		info->rotation_type = OMAP_DSS_ROT_NONE;
> -		info->rotation      = DRM_MODE_ROTATE_0;
> -		info->screen_width  = plane->pitch;
> +		main_info->paddr = get_linear_addr(plane, format, 0, x, y);
> +		main_info->rotation_type = OMAP_DSS_ROT_NONE;
> +		main_info->rotation = DRM_MODE_ROTATE_0;
> +		main_info->screen_width = plane->pitch;
>  	}
>  
>  	/* convert to pixels: */
> -	info->screen_width /= format->cpp[0];
> +	main_info->screen_width /= format->cpp[0];
>  
>  	if (fb->format->format == DRM_FORMAT_NV12) {
>  		plane = &omap_fb->planes[1];
>  
> -		if (info->rotation_type == OMAP_DSS_ROT_TILER) {
> +		if (main_info->rotation_type == OMAP_DSS_ROT_TILER) {
>  			WARN_ON(!(omap_gem_flags(plane->bo) & OMAP_BO_TILED));
>  			omap_gem_rotated_dma_addr(plane->bo, orient, x/2, y/2,
> -						  &info->p_uv_addr);
> +						  &main_info->p_uv_addr);
>  		} else {
> -			info->p_uv_addr = get_linear_addr(plane, format, 1, x, y);
> +			main_info->p_uv_addr =
> +				get_linear_addr(plane, format, 1, x, y);
>  		}
>  	} else {
> -		info->p_uv_addr = 0;
> +		main_info->p_uv_addr = 0;
> +	}
> +
> +	if (aux_info) {
> +		main_info->width /= 2;
> +		main_info->out_width /= 2;
> +
> +		*aux_info = *main_info;
> +
> +		aux_info->pos_x = main_info->pos_x + main_info->out_width;
> +
> +		aux_info->paddr =
> +				get_linear_addr(&omap_fb->planes[0], format, 0,
> +						x + main_info->width, y);
> +		if (fb->format->format == DRM_FORMAT_NV12) {
> +			aux_info->p_uv_addr =
> +				get_linear_addr(&omap_fb->planes[1], format, 1,
> +						x + main_info->width, y);
> +		}
>  	}
>  }
>  
> diff --git a/drivers/gpu/drm/omapdrm/omap_fb.h b/drivers/gpu/drm/omapdrm/omap_fb.h
> index 94ad5f9e4404..f68e81353288 100644
> --- a/drivers/gpu/drm/omapdrm/omap_fb.h
> +++ b/drivers/gpu/drm/omapdrm/omap_fb.h
> @@ -37,7 +37,9 @@ struct drm_framebuffer *omap_framebuffer_init(struct drm_device *dev,
>  int omap_framebuffer_pin(struct drm_framebuffer *fb);
>  void omap_framebuffer_unpin(struct drm_framebuffer *fb);
>  void omap_framebuffer_update_scanout(struct drm_framebuffer *fb,
> -		struct drm_plane_state *state, struct omap_overlay_info *info);
> +		struct drm_plane_state *state,
> +		struct omap_overlay_info *main_info,
> +		struct omap_overlay_info *aux_info);
>  struct drm_connector *omap_framebuffer_get_next_connector(
>  		struct drm_framebuffer *fb, struct drm_connector *from);
>  bool omap_framebuffer_supports_rotation(struct drm_framebuffer *fb);
> diff --git a/drivers/gpu/drm/omapdrm/omap_plane.c b/drivers/gpu/drm/omapdrm/omap_plane.c
> index 7d789d1551a1..e3e6623c405d 100644
> --- a/drivers/gpu/drm/omapdrm/omap_plane.c
> +++ b/drivers/gpu/drm/omapdrm/omap_plane.c
> @@ -30,10 +30,14 @@
>  
>  struct omap_plane {
>  	struct drm_plane base;
> -	enum omap_plane_id id;
> +	enum omap_plane_id main_id;
> +	enum omap_plane_id aux_id;
>  	const char *name;
> +	bool virtual_plane;
>  };
>  
> +static const char *plane_id_to_name[];
> +
>  static int omap_plane_prepare_fb(struct drm_plane *plane,
>  				 struct drm_plane_state *new_state)
>  {
> @@ -56,38 +60,70 @@ static void omap_plane_atomic_update(struct drm_plane *plane,
>  	struct omap_drm_private *priv = plane->dev->dev_private;
>  	struct omap_plane *omap_plane = to_omap_plane(plane);
>  	struct drm_plane_state *state = plane->state;
> -	struct omap_overlay_info info;
> +	struct omap_overlay_info main_info, aux_info;
>  	int ret;
> +	bool dual_plane = omap_plane->virtual_plane;
>  
>  	DBG("%s, crtc=%p fb=%p", omap_plane->name, state->crtc, state->fb);
>  
> -	memset(&info, 0, sizeof(info));
> -	info.rotation_type = OMAP_DSS_ROT_NONE;
> -	info.rotation = DRM_MODE_ROTATE_0;
> -	info.global_alpha = 0xff;
> -	info.zorder = state->zpos;
> +	memset(&main_info, 0, sizeof(main_info));
> +	main_info.rotation_type = OMAP_DSS_ROT_NONE;
> +	main_info.rotation = DRM_MODE_ROTATE_0;
> +	main_info.global_alpha = 0xff;
> +	main_info.zorder = state->zpos;
>  
> -	/* update scanout: */
> -	omap_framebuffer_update_scanout(state->fb, state, &info);
> +	aux_info = main_info;
>  
> -	DBG("%dx%d -> %dx%d (%d)", info.width, info.height,
> -			info.out_width, info.out_height,
> -			info.screen_width);
> -	DBG("%d,%d %pad %pad", info.pos_x, info.pos_y,
> -			&info.paddr, &info.p_uv_addr);
> +	/* update scanout: */
> +	omap_framebuffer_update_scanout(state->fb, state, &main_info,
> +					dual_plane ? &aux_info : NULL);
> +
> +	DBG("%s: %dx%d -> %dx%d (%d)",
> +	    plane_id_to_name[omap_plane->main_id],
> +	    main_info.width, main_info.height,
> +	    main_info.out_width, main_info.out_height,
> +	    main_info.screen_width);
> +	DBG("%d,%d %pad %pad", main_info.pos_x, main_info.pos_y,
> +	    &main_info.paddr, &main_info.p_uv_addr);
> +
> +	if (dual_plane) {
> +		aux_info.zorder = main_info.zorder + 1; // XXX

This is broken, and it's even marked as such with the XXX. For example,
this fails:

kmstest -c 0 -p 400x400 -Pzpos=0 -p 300x300 -Pzpos=1 -p 200x200 -Pzpos=2

 Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Patch 2/4] dt-bindings: display/ti: Add plane binding to dispc node

[Rob Herring]

On Wed, Mar 14, 2018 at 6:23 AM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> On 09/03/18 20:27, Benoit Parrot wrote:
>
>>> Is logical plane a h/w concept?
>>
>> It does represent a hardware resource.
>
> Logical plane is not a hw concept, it just describes a group of one or
> two HW planes. Then again, in the context of 2k+ displays, two HW planes
> must always be used together, so that way it could be considered a
> single HW resource.
>
>>> Really, I'm skeptical that any of this belongs in DT. For example,
>>> can't you figure out you need 2 physical planes whenever your
>>> panel/timing width is greater than 2048?
>>
>> As stated in the description I added above, we cannot have resources
>> exposed to user-space which can "disappear" dynamically.
>> Doing so would break user-space applications which rely on these
>> resources.

Isn't this the point of atomic mode setting? If you have 2 planes
free, then you can support the mode, otherwise you fail. How would a
plane be in use if you are doing modesetting unless it is on another
display?

> The question is, if not in DT, then where? I agree that this is not
> exactly describing the HW. But it can't be done dynamically either (or
> at least we have not figured out a way). And it must be user configurable.

If you are plugging in a monitor, doesn't it have to be dynamic?

> Module parameters are an option, but it would be somewhat difficult to
> give all this information there. And also, if your board has a 2k+
> display, you must have these configurations given to the driver, it's
> not optional.

Can't you look at the panel size on boot or module load and determine
if you need to combine planes or not. The main difference I see is
that the driver would have to figure out which planes to use rather
than DT telling it what planes to use. Is deciding which planes a hard
problem?

>
> And while it's perhaps stretching the definitions a bit, I guess one
> could argue that this describes the HW in a way: it describes how the HW
> resources must be used if you have a display of 2k+ width, and is not as
> such related to Linux or DRM.
>
>  Tomi
>
> --
> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
> Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Patch 2/4] dt-bindings: display/ti: Add plane binding to dispc node

[Tomi Valkeinen]

Hi Rob,

On 19/03/18 02:06, Rob Herring wrote:
> On Wed, Mar 14, 2018 at 6:23 AM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
>> On 09/03/18 20:27, Benoit Parrot wrote:
>>
>>>> Is logical plane a h/w concept?
>>>
>>> It does represent a hardware resource.
>>
>> Logical plane is not a hw concept, it just describes a group of one or
>> two HW planes. Then again, in the context of 2k+ displays, two HW planes
>> must always be used together, so that way it could be considered a
>> single HW resource.
>>
>>>> Really, I'm skeptical that any of this belongs in DT. For example,
>>>> can't you figure out you need 2 physical planes whenever your
>>>> panel/timing width is greater than 2048?
>>>
>>> As stated in the description I added above, we cannot have resources
>>> exposed to user-space which can "disappear" dynamically.
>>> Doing so would break user-space applications which rely on these
>>> resources.
> 
> Isn't this the point of atomic mode setting? If you have 2 planes
> free, then you can support the mode, otherwise you fail. How would a
> plane be in use if you are doing modesetting unless it is on another
> display?
> 
>> The question is, if not in DT, then where? I agree that this is not
>> exactly describing the HW. But it can't be done dynamically either (or
>> at least we have not figured out a way). And it must be user configurable.
> 
> If you are plugging in a monitor, doesn't it have to be dynamic?
> 
>> Module parameters are an option, but it would be somewhat difficult to
>> give all this information there. And also, if your board has a 2k+
>> display, you must have these configurations given to the driver, it's
>> not optional.
> 
> Can't you look at the panel size on boot or module load and determine
> if you need to combine planes or not. The main difference I see is
> that the driver would have to figure out which planes to use rather
> than DT telling it what planes to use. Is deciding which planes a hard
> problem?

Ok, I think the description was a bit unclear. So, the driver can do
this just fine, it can reserve hw planes dynamically when needed. The
problem is the userspace.

When a DRM application starts, it sees a bunch of planes, and can see on
which crtcs each plane can be used. The expectation is, of course, that
these planes can be used normally. If the driver would dynamically
reserve an additional, currently unused plane, the userspace would be
totally baffled, as it fails to configure basic plane setups.

For example, the userspace could see that there are two planes, usable
on LCD and HDMI crtcs. But mysteriously modesetting would sometimes fail
if the HDMI is 2k+ display. Setting up a plane on the HDMI would work,
except when the LCD already has a plane. Setting up two planes on the
LCD would work, but moving one or both planes to the HDMI would fail. Etc.

We could, of course, convey this information to the userspace at runtime
via the DRM properties, but then it would mean we'd need customized
applications.

So, as far as I can see, keeping normal DRM behavior with 2k+ displays
on OMAP DSS requires a static virtual plane setup. The most simple setup
would be to just split the number of available planes by 2, but then in
many use cases that wastes one hw plane.

 Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Patch 2/4] dt-bindings: display/ti: Add plane binding to dispc node

[Rob Herring]

On Mon, Mar 19, 2018 at 2:15 AM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> Hi Rob,
>
> On 19/03/18 02:06, Rob Herring wrote:
>> On Wed, Mar 14, 2018 at 6:23 AM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
>>> On 09/03/18 20:27, Benoit Parrot wrote:
>>>
>>>>> Is logical plane a h/w concept?
>>>>
>>>> It does represent a hardware resource.
>>>
>>> Logical plane is not a hw concept, it just describes a group of one or
>>> two HW planes. Then again, in the context of 2k+ displays, two HW planes
>>> must always be used together, so that way it could be considered a
>>> single HW resource.
>>>
>>>>> Really, I'm skeptical that any of this belongs in DT. For example,
>>>>> can't you figure out you need 2 physical planes whenever your
>>>>> panel/timing width is greater than 2048?
>>>>
>>>> As stated in the description I added above, we cannot have resources
>>>> exposed to user-space which can "disappear" dynamically.
>>>> Doing so would break user-space applications which rely on these
>>>> resources.
>>
>> Isn't this the point of atomic mode setting? If you have 2 planes
>> free, then you can support the mode, otherwise you fail. How would a
>> plane be in use if you are doing modesetting unless it is on another
>> display?
>>
>>> The question is, if not in DT, then where? I agree that this is not
>>> exactly describing the HW. But it can't be done dynamically either (or
>>> at least we have not figured out a way). And it must be user configurable.
>>
>> If you are plugging in a monitor, doesn't it have to be dynamic?
>>
>>> Module parameters are an option, but it would be somewhat difficult to
>>> give all this information there. And also, if your board has a 2k+
>>> display, you must have these configurations given to the driver, it's
>>> not optional.
>>
>> Can't you look at the panel size on boot or module load and determine
>> if you need to combine planes or not. The main difference I see is
>> that the driver would have to figure out which planes to use rather
>> than DT telling it what planes to use. Is deciding which planes a hard
>> problem?
>
> Ok, I think the description was a bit unclear. So, the driver can do
> this just fine, it can reserve hw planes dynamically when needed. The
> problem is the userspace.
>
> When a DRM application starts, it sees a bunch of planes, and can see on
> which crtcs each plane can be used. The expectation is, of course, that
> these planes can be used normally. If the driver would dynamically
> reserve an additional, currently unused plane, the userspace would be
> totally baffled, as it fails to configure basic plane setups.
>
> For example, the userspace could see that there are two planes, usable
> on LCD and HDMI crtcs. But mysteriously modesetting would sometimes fail
> if the HDMI is 2k+ display. Setting up a plane on the HDMI would work,
> except when the LCD already has a plane. Setting up two planes on the
> LCD would work, but moving one or both planes to the HDMI would fail. Etc.

I suspect this is a common problem. Not because the h/w requires
different allocation of planes, but because the memory bandwidth can't
handle having a 2nd plane if the resolution is above a certain
size/depth. So while the plane doesn't disappear, the effect is the
same. How does DRM handle this?

> We could, of course, convey this information to the userspace at runtime
> via the DRM properties, but then it would mean we'd need customized
> applications.
>
> So, as far as I can see, keeping normal DRM behavior with 2k+ displays
> on OMAP DSS requires a static virtual plane setup. The most simple setup
> would be to just split the number of available planes by 2, but then in
> many use cases that wastes one hw plane.

For HDMI, you can't know in advance what resolution will be. So I
think you always need to reserve 2 planes. Now, if you want to reduce
the max resolution for some reason, I guess we could have properties
for that. That would be more generic and work whether you need to
change plane allocation or have a limit for other reasons.

For attached panels, you know the resolution up front and can allocate
planes before the userspace interface is up.

Rob
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Patch 2/4] dt-bindings: display/ti: Add plane binding to dispc node

[Tomi Valkeinen]

Hi Rob,

On 23/03/18 03:23, Rob Herring wrote:

>> Ok, I think the description was a bit unclear. So, the driver can do
>> this just fine, it can reserve hw planes dynamically when needed. The
>> problem is the userspace.
>>
>> When a DRM application starts, it sees a bunch of planes, and can see on
>> which crtcs each plane can be used. The expectation is, of course, that
>> these planes can be used normally. If the driver would dynamically
>> reserve an additional, currently unused plane, the userspace would be
>> totally baffled, as it fails to configure basic plane setups.
>>
>> For example, the userspace could see that there are two planes, usable
>> on LCD and HDMI crtcs. But mysteriously modesetting would sometimes fail
>> if the HDMI is 2k+ display. Setting up a plane on the HDMI would work,
>> except when the LCD already has a plane. Setting up two planes on the
>> LCD would work, but moving one or both planes to the HDMI would fail. Etc.
> 
> I suspect this is a common problem. Not because the h/w requires
> different allocation of planes, but because the memory bandwidth can't
> handle having a 2nd plane if the resolution is above a certain
> size/depth. So while the plane doesn't disappear, the effect is the
> same. How does DRM handle this?

I don't think DRM handles this. Each driver can probably filter out
videomodes which it knows can't be used even with single plane (we do
this on omapdrm), and also can give an error if the plane setup would
result in too high bandwidth use.

So yes, plane setups can always fail, "mysteriously" from userspace's
perspective. But I don't think it's exactly comparable to this one. The
difference is that in this case we can avoid all the userspace issues
with a simple static plane partitioning done at probe time, but I can't
see how the bandwidth issue could be solved in a similar way.

>> We could, of course, convey this information to the userspace at runtime
>> via the DRM properties, but then it would mean we'd need customized
>> applications.
>>
>> So, as far as I can see, keeping normal DRM behavior with 2k+ displays
>> on OMAP DSS requires a static virtual plane setup. The most simple setup
>> would be to just split the number of available planes by 2, but then in
>> many use cases that wastes one hw plane.
> 
> For HDMI, you can't know in advance what resolution will be. So I
> think you always need to reserve 2 planes. Now, if you want to reduce

We can decide not to support 2k+ resolutions for HDMI, which, with this
series, happens by not reserving dual-plane for the HDMI.

> the max resolution for some reason, I guess we could have properties
> for that. That would be more generic and work whether you need to
> change plane allocation or have a limit for other reasons.
> 
> For attached panels, you know the resolution up front and can allocate
> planes before the userspace interface is up.

But reserve how many of the planes? We have N planes and M displays. For
some of the displays we know they're 2k+, some are known to be -2k and
some are unknown. The driver can't independently make any sensible
static reservation of the planes for the displays, because it doesn't
know what the user wants to do.

So either we reserve the extra planes at runtime on demand, making it
difficult to manage for the userspace, or we rely on the user to give
the driver a static partitioning of the planes according to the user's
use case.

 Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Patch 2/4] dt-bindings: display/ti: Add plane binding to dispc node

[Rob Herring]

On Fri, Mar 23, 2018 at 2:53 AM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> Hi Rob,
>
> On 23/03/18 03:23, Rob Herring wrote:
>
>>> Ok, I think the description was a bit unclear. So, the driver can do
>>> this just fine, it can reserve hw planes dynamically when needed. The
>>> problem is the userspace.
>>>
>>> When a DRM application starts, it sees a bunch of planes, and can see on
>>> which crtcs each plane can be used. The expectation is, of course, that
>>> these planes can be used normally. If the driver would dynamically
>>> reserve an additional, currently unused plane, the userspace would be
>>> totally baffled, as it fails to configure basic plane setups.
>>>
>>> For example, the userspace could see that there are two planes, usable
>>> on LCD and HDMI crtcs. But mysteriously modesetting would sometimes fail
>>> if the HDMI is 2k+ display. Setting up a plane on the HDMI would work,
>>> except when the LCD already has a plane. Setting up two planes on the
>>> LCD would work, but moving one or both planes to the HDMI would fail. Etc.
>>
>> I suspect this is a common problem. Not because the h/w requires
>> different allocation of planes, but because the memory bandwidth can't
>> handle having a 2nd plane if the resolution is above a certain
>> size/depth. So while the plane doesn't disappear, the effect is the
>> same. How does DRM handle this?
>
> I don't think DRM handles this. Each driver can probably filter out
> videomodes which it knows can't be used even with single plane (we do
> this on omapdrm), and also can give an error if the plane setup would
> result in too high bandwidth use.
>
> So yes, plane setups can always fail, "mysteriously" from userspace's
> perspective. But I don't think it's exactly comparable to this one. The
> difference is that in this case we can avoid all the userspace issues
> with a simple static plane partitioning done at probe time, but I can't
> see how the bandwidth issue could be solved in a similar way.
>
>>> We could, of course, convey this information to the userspace at runtime
>>> via the DRM properties, but then it would mean we'd need customized
>>> applications.
>>>
>>> So, as far as I can see, keeping normal DRM behavior with 2k+ displays
>>> on OMAP DSS requires a static virtual plane setup. The most simple setup
>>> would be to just split the number of available planes by 2, but then in
>>> many use cases that wastes one hw plane.
>>
>> For HDMI, you can't know in advance what resolution will be. So I
>> think you always need to reserve 2 planes. Now, if you want to reduce
>
> We can decide not to support 2k+ resolutions for HDMI, which, with this
> series, happens by not reserving dual-plane for the HDMI.

Right. So turn this around. Define in DT what is the maximum
resolution supported for HDMI and configure the planes based on that.
The difference is defining a max is generic enough that it can work
for others and for a variety of reasons whether it is # of planes,
memory bandwidth, crappy monitor, poor board signal quality, etc.

>> the max resolution for some reason, I guess we could have properties
>> for that. That would be more generic and work whether you need to
>> change plane allocation or have a limit for other reasons.
>>
>> For attached panels, you know the resolution up front and can allocate
>> planes before the userspace interface is up.
>
> But reserve how many of the planes? We have N planes and M displays. For
> some of the displays we know they're 2k+, some are known to be -2k and
> some are unknown. The driver can't independently make any sensible
> static reservation of the planes for the displays, because it doesn't
> know what the user wants to do.

After you've handled HDMI as above and any permanently attached panels
with fixed resolutions, what is left for a user to configure? Perhaps
only one display can support an overlay at that point because you are
out of planes?

> So either we reserve the extra planes at runtime on demand, making it
> difficult to manage for the userspace, or we rely on the user to give
> the driver a static partitioning of the planes according to the user's
> use case.

And by user, who do you mean exactly? The use case is tied to the
board design and product or tied to the whims of an end user (e.g. I
want to do video playback with overlay to disp 2)? You should equate
users making DT changes with telling users to update/change their
BIOS.

Rob
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Patch 2/4] dt-bindings: display/ti: Add plane binding to dispc node

[Tomi Valkeinen]

Hi Rob,

On 09/04/18 21:17, Rob Herring wrote:

>>> For HDMI, you can't know in advance what resolution will be. So I
>>> think you always need to reserve 2 planes. Now, if you want to reduce
>>
>> We can decide not to support 2k+ resolutions for HDMI, which, with this
>> series, happens by not reserving dual-plane for the HDMI.
> 
> Right. So turn this around. Define in DT what is the maximum
> resolution supported for HDMI and configure the planes based on that.

But the kernel cannot know what the user wants to do, so it cannot
configure the planes. If we have an HDMI output which supports 2k+ and a
-2k LCD, and 4 hw planes, we can set up the planes at least in the
following ways:

- One virtual plane on HDMI, two normal planes on LCD. Here the normal
planes can also be used on the HDMI, as long as the input width is -2k.

- One virtual plane on HDMI, one virtual plane on LCD, but sometimes
both planes on the same display (either HDMI or LCD).

- No virtual planes (and fbdev support disabled). This needs the
userspace to take care not to configure 2k+ planes. But considering that
the modes supported are still quit close to 2k in width, I believe
upscaling a 2k plane cover the whole display would provide quite ok image.

Each of those requires a different virtual plane setup.

>> But reserve how many of the planes? We have N planes and M displays. For
>> some of the displays we know they're 2k+, some are known to be -2k and
>> some are unknown. The driver can't independently make any sensible
>> static reservation of the planes for the displays, because it doesn't
>> know what the user wants to do.
> 
> After you've handled HDMI as above and any permanently attached panels
> with fixed resolutions, what is left for a user to configure? Perhaps
> only one display can support an overlay at that point because you are
> out of planes?

I think I covered this in the example use cases above.

>> So either we reserve the extra planes at runtime on demand, making it
>> difficult to manage for the userspace, or we rely on the user to give
>> the driver a static partitioning of the planes according to the user's
>> use case.
> 
> And by user, who do you mean exactly? The use case is tied to the
> board design and product or tied to the whims of an end user (e.g. I
> want to do video playback with overlay to disp 2)? You should equate
> users making DT changes with telling users to update/change their
> BIOS.

By user I mean the owner of the device, but it in some cases it could be
the vendor too.

If we have a board with HDMI that can support 2k+, then the board vendor
could provide DT files that do not specify virtual planes (so no 2k+),
but give instructions how to define them for the users who want 2k+. So
here the end user needs to deal with the static partitioning.

If we have a board with a fixed resolution 2k+ LCD, then the vendor has
to have a virtual plane defined in the DT, and the vendor has to pick a
configuration that it thinks is most useful.

And yes, it sucks to have to make changes in the DT or BIOS, but I still
don't see a (good) alternative.

I think one option is to have the detailed DT configuration optional:

We'd have a flag in the DT to mark that a display supports 2k+ (or the
max-resolution property as you suggested). Based on that, the driver
guesses what kind of setup the user wants, which probably is just to set
up planes in a way that each display has a fully functional "root"
plane, and then split the remaining ones in some way.

But the user could also have detailed DT descriptions for the planes
when he needs a more special setup.

 Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Patch 2/4] dt-bindings: display/ti: Add plane binding to dispc node

[Daniel Vetter]

On Tue, Apr 17, 2018 at 05:37:25PM +0300, Tomi Valkeinen wrote:
> Hi Rob,
> 
> On 09/04/18 21:17, Rob Herring wrote:
> 
> >>> For HDMI, you can't know in advance what resolution will be. So I
> >>> think you always need to reserve 2 planes. Now, if you want to reduce
> >>
> >> We can decide not to support 2k+ resolutions for HDMI, which, with this
> >> series, happens by not reserving dual-plane for the HDMI.
> > 
> > Right. So turn this around. Define in DT what is the maximum
> > resolution supported for HDMI and configure the planes based on that.
> 
> But the kernel cannot know what the user wants to do, so it cannot
> configure the planes. If we have an HDMI output which supports 2k+ and a
> -2k LCD, and 4 hw planes, we can set up the planes at least in the
> following ways:
> 
> - One virtual plane on HDMI, two normal planes on LCD. Here the normal
> planes can also be used on the HDMI, as long as the input width is -2k.
> 
> - One virtual plane on HDMI, one virtual plane on LCD, but sometimes
> both planes on the same display (either HDMI or LCD).
> 
> - No virtual planes (and fbdev support disabled). This needs the
> userspace to take care not to configure 2k+ planes. But considering that
> the modes supported are still quit close to 2k in width, I believe
> upscaling a 2k plane cover the whole display would provide quite ok image.
> 
> Each of those requires a different virtual plane setup.

Why do you want to hardcode this in DT? The recommended approach is to
have a bunch of virtual planes, and runtime assign whatever hw resources
you need to get there. If you run out of hw planes you just fail with
-EINVAL in atomic_check.

And reassigning hw planes is allowed to be really expensive, that's why we
have the ALLOW_MODESET flag so userspace can choose whether it wants to
allow expensive reassignment or not.

For examples, see what msm folks are trying to do right now.
-Daniel

> >> But reserve how many of the planes? We have N planes and M displays. For
> >> some of the displays we know they're 2k+, some are known to be -2k and
> >> some are unknown. The driver can't independently make any sensible
> >> static reservation of the planes for the displays, because it doesn't
> >> know what the user wants to do.
> > 
> > After you've handled HDMI as above and any permanently attached panels
> > with fixed resolutions, what is left for a user to configure? Perhaps
> > only one display can support an overlay at that point because you are
> > out of planes?
> 
> I think I covered this in the example use cases above.
> 
> >> So either we reserve the extra planes at runtime on demand, making it
> >> difficult to manage for the userspace, or we rely on the user to give
> >> the driver a static partitioning of the planes according to the user's
> >> use case.
> > 
> > And by user, who do you mean exactly? The use case is tied to the
> > board design and product or tied to the whims of an end user (e.g. I
> > want to do video playback with overlay to disp 2)? You should equate
> > users making DT changes with telling users to update/change their
> > BIOS.
> 
> By user I mean the owner of the device, but it in some cases it could be
> the vendor too.
> 
> If we have a board with HDMI that can support 2k+, then the board vendor
> could provide DT files that do not specify virtual planes (so no 2k+),
> but give instructions how to define them for the users who want 2k+. So
> here the end user needs to deal with the static partitioning.
> 
> If we have a board with a fixed resolution 2k+ LCD, then the vendor has
> to have a virtual plane defined in the DT, and the vendor has to pick a
> configuration that it thinks is most useful.
> 
> And yes, it sucks to have to make changes in the DT or BIOS, but I still
> don't see a (good) alternative.
> 
> I think one option is to have the detailed DT configuration optional:
> 
> We'd have a flag in the DT to mark that a display supports 2k+ (or the
> max-resolution property as you suggested). Based on that, the driver
> guesses what kind of setup the user wants, which probably is just to set
> up planes in a way that each display has a fully functional "root"
> plane, and then split the remaining ones in some way.
> 
> But the user could also have detailed DT descriptions for the planes
> when he needs a more special setup.
> 
>  Tomi
> 
> -- 
> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
> Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Patch 2/4] dt-bindings: display/ti: Add plane binding to dispc node

[Tomi Valkeinen]

On 19/04/18 09:34, Daniel Vetter wrote:

>> But the kernel cannot know what the user wants to do, so it cannot
>> configure the planes. If we have an HDMI output which supports 2k+ and a
>> -2k LCD, and 4 hw planes, we can set up the planes at least in the
>> following ways:
>>
>> - One virtual plane on HDMI, two normal planes on LCD. Here the normal
>> planes can also be used on the HDMI, as long as the input width is -2k.
>>
>> - One virtual plane on HDMI, one virtual plane on LCD, but sometimes
>> both planes on the same display (either HDMI or LCD).
>>
>> - No virtual planes (and fbdev support disabled). This needs the
>> userspace to take care not to configure 2k+ planes. But considering that
>> the modes supported are still quit close to 2k in width, I believe
>> upscaling a 2k plane cover the whole display would provide quite ok image.
>>
>> Each of those requires a different virtual plane setup.
> 
> Why do you want to hardcode this in DT? The recommended approach is to
> have a bunch of virtual planes, and runtime assign whatever hw resources
> you need to get there. If you run out of hw planes you just fail with
> -EINVAL in atomic_check.

That is possible, but how many userspace apps will work correctly if the
planes work or don't work in a random manner (from userspace's point of
view)? I like the idea more that the DRM driver exposes a lesser number
of planes which always work, instead of exposing larger number of planes
which sometimes do not work.

And with userspace apps, I don't mainly mean Weston and X, but instead
the numerous custom applications the customers write themselves. Perhaps
I'm worrying too much, but I can imagine a flood of support requests
about why plane setup is not working when one does this or that simple
setup.

Also one complication with runtime assignment is that the hw planes are
not identical: some support YUV modes, some don't, some support scaling,
some don't. That's probably not a show stopper, but it does limit the
options as e.g. we can't have all virtual planes advertising YUV support
when we have a hw plane that doesn't support YUV.

 Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Patch 2/4] dt-bindings: display/ti: Add plane binding to dispc node

[Daniel Vetter]

On Thu, Apr 19, 2018 at 10:11:05AM +0300, Tomi Valkeinen wrote:
> On 19/04/18 09:34, Daniel Vetter wrote:
> 
> >> But the kernel cannot know what the user wants to do, so it cannot
> >> configure the planes. If we have an HDMI output which supports 2k+ and a
> >> -2k LCD, and 4 hw planes, we can set up the planes at least in the
> >> following ways:
> >>
> >> - One virtual plane on HDMI, two normal planes on LCD. Here the normal
> >> planes can also be used on the HDMI, as long as the input width is -2k.
> >>
> >> - One virtual plane on HDMI, one virtual plane on LCD, but sometimes
> >> both planes on the same display (either HDMI or LCD).
> >>
> >> - No virtual planes (and fbdev support disabled). This needs the
> >> userspace to take care not to configure 2k+ planes. But considering that
> >> the modes supported are still quit close to 2k in width, I believe
> >> upscaling a 2k plane cover the whole display would provide quite ok image.
> >>
> >> Each of those requires a different virtual plane setup.
> > 
> > Why do you want to hardcode this in DT? The recommended approach is to
> > have a bunch of virtual planes, and runtime assign whatever hw resources
> > you need to get there. If you run out of hw planes you just fail with
> > -EINVAL in atomic_check.
> 
> That is possible, but how many userspace apps will work correctly if the
> planes work or don't work in a random manner (from userspace's point of
> view)? I like the idea more that the DRM driver exposes a lesser number
> of planes which always work, instead of exposing larger number of planes
> which sometimes do not work.
> 
> And with userspace apps, I don't mainly mean Weston and X, but instead
> the numerous custom applications the customers write themselves. Perhaps
> I'm worrying too much, but I can imagine a flood of support requests
> about why plane setup is not working when one does this or that simple
> setup.

Stuff randomly not working is officially how atomic works. That's what the
TEST_ONLY mode is for. There's a lot more than virtualized planes that
might or might not push any given plane setup over some random hw limit:
memory bandwidth, aggregate scaling limits (because not enough fifo),
thermal limits, aggregate pixel clock limits. There's all kinds of cases
where with one setup you can light up 4 planes, then move them a bit and
only 3 work. And yes sometimes that means you can't light up all the
outputs if you have a too fancy plane config on the other CRTC.

Only userspace which is written with intimate knowledge of the exact
kernel driver and hw it runs on can avoid TEST_ONLY and some kind of
fallback strategy to "render everything into the 1 single primary plane".
Both drm_hwcomposer and weston atomic have such a fallback strategy (with
various degrees of intermediate cleverness).

> Also one complication with runtime assignment is that the hw planes are
> not identical: some support YUV modes, some don't, some support scaling,
> some don't. That's probably not a show stopper, but it does limit the
> options as e.g. we can't have all virtual planes advertising YUV support
> when we have a hw plane that doesn't support YUV.

Yeah that makes it a notch more complicated to implement.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Patch 2/4] dt-bindings: display/ti: Add plane binding to dispc node

[Tomi Valkeinen]

On 20/04/18 10:00, Daniel Vetter wrote:

(Adding Benoit back, he was dropped at some point in the thread)

> Stuff randomly not working is officially how atomic works. That's what the
> TEST_ONLY mode is for. There's a lot more than virtualized planes that
> might or might not push any given plane setup over some random hw limit:
> memory bandwidth, aggregate scaling limits (because not enough fifo),
> thermal limits, aggregate pixel clock limits. There's all kinds of cases
> where with one setup you can light up 4 planes, then move them a bit and
> only 3 work. And yes sometimes that means you can't light up all the
> outputs if you have a too fancy plane config on the other CRTC.
> 
> Only userspace which is written with intimate knowledge of the exact
> kernel driver and hw it runs on can avoid TEST_ONLY and some kind of
> fallback strategy to "render everything into the 1 single primary plane".
> Both drm_hwcomposer and weston atomic have such a fallback strategy (with
> various degrees of intermediate cleverness).

Ok, thanks. This makes sense to me, and actually makes our (driver
developers) life easier... Is "Stuff randomly not working is officially
how atomic works." mentioned in the DRM documentation? I think that
sentence summarizes it quite well =).

Does the driver still have some minimal set of functionality it always
has to allow? E.g. is enabling all the available displays with the
display's native resolution (or whatever passes the mode_valid), each
with a single non-scaled full-screen primary plane, something every
driver should ensure is always possible?

 Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Patch 2/4] dt-bindings: display/ti: Add plane binding to dispc node

[Daniel Vetter]

On Fri, Apr 20, 2018 at 10:21:35AM +0300, Tomi Valkeinen wrote:
> On 20/04/18 10:00, Daniel Vetter wrote:
> 
> (Adding Benoit back, he was dropped at some point in the thread)
> 
> > Stuff randomly not working is officially how atomic works. That's what the
> > TEST_ONLY mode is for. There's a lot more than virtualized planes that
> > might or might not push any given plane setup over some random hw limit:
> > memory bandwidth, aggregate scaling limits (because not enough fifo),
> > thermal limits, aggregate pixel clock limits. There's all kinds of cases
> > where with one setup you can light up 4 planes, then move them a bit and
> > only 3 work. And yes sometimes that means you can't light up all the
> > outputs if you have a too fancy plane config on the other CRTC.
> > 
> > Only userspace which is written with intimate knowledge of the exact
> > kernel driver and hw it runs on can avoid TEST_ONLY and some kind of
> > fallback strategy to "render everything into the 1 single primary plane".
> > Both drm_hwcomposer and weston atomic have such a fallback strategy (with
> > various degrees of intermediate cleverness).
> 
> Ok, thanks. This makes sense to me, and actually makes our (driver
> developers) life easier... Is "Stuff randomly not working is officially
> how atomic works." mentioned in the DRM documentation? I think that
> sentence summarizes it quite well =).

Hm, I think we fail to doc that properly, at least in-kernel. Improving
atomic docs is somewhere on my huge todo list still :-/

The lwn article I've written does explain the motivation behind TEST_ONLY
in detail though:

https://lwn.net/Articles/653071/

> Does the driver still have some minimal set of functionality it always
> has to allow? E.g. is enabling all the available displays with the
> display's native resolution (or whatever passes the mode_valid), each
> with a single non-scaled full-screen primary plane, something every
> driver should ensure is always possible?

Strive for, sure. "Always" is a bit much since you easily might run out of
clocks (if they all run at different modes), or memmory bandwidth (if
there's too many 8k screens) and stuff like that. It's really all best
effort.

The only guarantee we give for mode_valid is that you should be able to
light this mode up if
- no other CRTC enabled,
- only using the primary plane,
- and you tried all the pixel formats of that plane (in case xrbg8888 is
  too much you might need to try out rgb565, or *shock* C8). I think in
  practice just trying xrbg8888 should be good enough, since that's what
  plymouth and other simple generic userspace requires. You're running on
  some really funky hw if that doesn't work.

Yes that guarantee is very minimal, but it's the best we can do really.

Historical note aside: The above problem is why atomic's TEST_ONLY also
works for modesets, and why atomic does updates across multiple CRTC:
Intel's gen7 was the first popular platform that had 3 pipe support but in
many situations only allowed to light up 2 outputs, not 3. Userspace died
to no end on failed modesets :-/
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel