[PATCH v6 0/9] drm/omap: Add virtual-planes support

[PATCH v6 0/9] drm/omap: Add virtual-planes support

[Neil Armstrong]

This patchset is the follow-up the v4 patchset from Benoit Parrot at [1].

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

In order to do so we introduce the concept of hw_overlay which can then be
dynamically allocated to a plane. When the requested output width exceed what
be supported by one overlay a second is then allocated if possible to handle
display wider then 2048.

This series replaces an earlier series which was DT based and using statically
allocated resources.

This implementation is inspired from the work done in msm/disp/mdp5
driver.

Changes since v5 at [2]:
- Patch 1: renamed width/height_fp to max_width/height
- Patch 2: no changes
- Patch 3: removed possible_crtcs stuff,
	added cleanup on failure to allocate,
	removed name in omap_plane struct & plane_id_to_name in omap_plane.c,
	switched all omap_plane->name to plane->name or omap_plane->id
- Patch 4: aligned omap_plane_atomic_duplicate_state the the crtc style
- Patch 5: removed glob_obj_lock & reformated global state declaration in omap_drv.h
- Patch 6: moved drm_atomic_helper_check_plane_state() from atomic_check() in separate commit,
	removed zpos change, updated debug messages to be useful,
	renamed omap_overlay_disable() to omap_overlay_update_state(),
	added useful comments for omap_overlay_assign() & omap_overlay_update_state(),
	simplified omap_overlay_assign() & omap_overlay_update_state() for actual use-cases,
	refactored omap_plane_atomic_check() changes to be cleaner & simpler
- Patch 7: no changes (except possible_crtcs print removal)
- Patch 8: Reformated omap_plane_atomic_check() & omap_overlay_assign() changes to match previous patches layout 

Changes since v4 at [1]:
- rebased on v5.15-rc2
- adapted to drm_atomic_get_new/old_plane_state()
- tested on Beagle-x15
- checked for non-regression on Beagle-x15
- removed unused "state" variable in omap_global_state

[1] https://lore.kernel.org/all/20181012201703.29065-1-bparrot@ti.com/
[2] https://lore.kernel.org/all/20210923070701.145377-1-narmstrong@baylibre.com/

Benoit Parrot (8):
  drm/omap: Add ability to check if requested plane modes can be
    supported
  drm/omap: Add ovl checking funcs to dispc_ops
  drm/omap: introduce omap_hw_overlay
  drm/omap: omap_plane: subclass drm_plane_state
  drm/omap: Add global state as a private atomic object
  drm/omap: dynamically assign hw overlays to planes
  drm/omap: add plane_atomic_print_state support
  drm/omap: Add a 'right overlay' to plane state

Neil Armstrong (1):
  drm/omap: add sanity plane state check

 drivers/gpu/drm/omapdrm/Makefile       |   1 +
 drivers/gpu/drm/omapdrm/dss/dispc.c    |  31 ++-
 drivers/gpu/drm/omapdrm/dss/dss.h      |   5 +
 drivers/gpu/drm/omapdrm/omap_drv.c     | 189 ++++++++++++-
 drivers/gpu/drm/omapdrm/omap_drv.h     |  24 ++
 drivers/gpu/drm/omapdrm/omap_fb.c      |  33 ++-
 drivers/gpu/drm/omapdrm/omap_fb.h      |   4 +-
 drivers/gpu/drm/omapdrm/omap_overlay.c | 212 +++++++++++++++
 drivers/gpu/drm/omapdrm/omap_overlay.h |  35 +++
 drivers/gpu/drm/omapdrm/omap_plane.c   | 354 +++++++++++++++++++++----
 drivers/gpu/drm/omapdrm/omap_plane.h   |   1 +
 11 files changed, 830 insertions(+), 59 deletions(-)
 create mode 100644 drivers/gpu/drm/omapdrm/omap_overlay.c
 create mode 100644 drivers/gpu/drm/omapdrm/omap_overlay.h


base-commit: e4e737bb5c170df6135a127739a9e6148ee3da82
-- 
2.25.1

[PATCH v6 1/9] drm/omap: add sanity plane state check

[Neil Armstrong]

Call drm_atomic_helper_check_plane_state() from the plane
atomic_check() callback in order to add plane state sanity
checking.

It will permit filtering out totally bad scaling factors, even
if the real check are done later in the atomic commit.

Signed-off-by: Benoit Parrot <bparrot@ti.com>
Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
---
 drivers/gpu/drm/omapdrm/omap_plane.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/gpu/drm/omapdrm/omap_plane.c b/drivers/gpu/drm/omapdrm/omap_plane.c
index 512af976b7e9..c3de4f339387 100644
--- a/drivers/gpu/drm/omapdrm/omap_plane.c
+++ b/drivers/gpu/drm/omapdrm/omap_plane.c
@@ -104,12 +104,15 @@ static void omap_plane_atomic_disable(struct drm_plane *plane,
 	dispc_ovl_enable(priv->dispc, omap_plane->id, false);
 }
 
+#define FRAC_16_16(mult, div)    (((mult) << 16) / (div))
+
 static int omap_plane_atomic_check(struct drm_plane *plane,
 				   struct drm_atomic_state *state)
 {
 	struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(state,
 										 plane);
 	struct drm_crtc_state *crtc_state;
+	int ret;
 
 	if (!new_plane_state->fb)
 		return 0;
@@ -127,6 +130,18 @@ static int omap_plane_atomic_check(struct drm_plane *plane,
 	if (!crtc_state->enable)
 		return 0;
 
+	/*
+	 * Note: these are just sanity checks to filter out totally bad scaling
+	 * factors. The real limits must be calculated case by case, and
+	 * unfortunately we currently do those checks only at the commit
+	 * phase in dispc.
+	 */
+	ret = drm_atomic_helper_check_plane_state(new_plane_state, crtc_state,
+						  FRAC_16_16(1, 8), FRAC_16_16(8, 1),
+						  true, true);
+	if (ret)
+		return ret;
+
 	if (new_plane_state->crtc_x < 0 || new_plane_state->crtc_y < 0)
 		return -EINVAL;
 
-- 
2.25.1

[PATCH v6 2/9] drm/omap: Add ability to check if requested plane modes can be supported

[Neil Armstrong]

From: Benoit Parrot <bparrot@ti.com>

We currently assume that an overlay has the same maximum width and
maximum height as the overlay manager. This assumption is incorrect. On
some variants the overlay manager maximum width is twice the maximum
width that the overlay can handle. We need to add the appropriate data
per variant as well as export a helper function to retrieve the data so
check can be made dynamically in omap_plane_atomic_check().

Signed-off-by: Benoit Parrot <bparrot@ti.com>
Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
---
 drivers/gpu/drm/omapdrm/dss/dispc.c  | 22 ++++++++++++++++++++++
 drivers/gpu/drm/omapdrm/dss/dss.h    |  2 ++
 drivers/gpu/drm/omapdrm/omap_plane.c | 14 ++++++++++++++
 3 files changed, 38 insertions(+)

diff --git a/drivers/gpu/drm/omapdrm/dss/dispc.c b/drivers/gpu/drm/omapdrm/dss/dispc.c
index 5619420cc2cc..5ac90ba6b76d 100644
--- a/drivers/gpu/drm/omapdrm/dss/dispc.c
+++ b/drivers/gpu/drm/omapdrm/dss/dispc.c
@@ -92,6 +92,8 @@ struct dispc_features {
 	u8 mgr_height_start;
 	u16 mgr_width_max;
 	u16 mgr_height_max;
+	u16 ovl_width_max;
+	u16 ovl_height_max;
 	unsigned long max_lcd_pclk;
 	unsigned long max_tv_pclk;
 	unsigned int max_downscale;
@@ -2599,6 +2601,12 @@ static int dispc_ovl_calc_scaling(struct dispc_device *dispc,
 	return 0;
 }
 
+void dispc_ovl_get_max_size(struct dispc_device *dispc, u16 *width, u16 *height)
+{
+	*width = dispc->feat->ovl_width_max;
+	*height = dispc->feat->ovl_height_max;
+}
+
 static int dispc_ovl_setup_common(struct dispc_device *dispc,
 				  enum omap_plane_id plane,
 				  enum omap_overlay_caps caps,
@@ -4240,6 +4248,8 @@ static const struct dispc_features omap24xx_dispc_feats = {
 	.mgr_height_start	=	26,
 	.mgr_width_max		=	2048,
 	.mgr_height_max		=	2048,
+	.ovl_width_max		=	2048,
+	.ovl_height_max		=	2048,
 	.max_lcd_pclk		=	66500000,
 	.max_downscale		=	2,
 	/*
@@ -4278,6 +4288,8 @@ static const struct dispc_features omap34xx_rev1_0_dispc_feats = {
 	.mgr_height_start	=	26,
 	.mgr_width_max		=	2048,
 	.mgr_height_max		=	2048,
+	.ovl_width_max		=	2048,
+	.ovl_height_max		=	2048,
 	.max_lcd_pclk		=	173000000,
 	.max_tv_pclk		=	59000000,
 	.max_downscale		=	4,
@@ -4313,6 +4325,8 @@ static const struct dispc_features omap34xx_rev3_0_dispc_feats = {
 	.mgr_height_start	=	26,
 	.mgr_width_max		=	2048,
 	.mgr_height_max		=	2048,
+	.ovl_width_max		=	2048,
+	.ovl_height_max		=	2048,
 	.max_lcd_pclk		=	173000000,
 	.max_tv_pclk		=	59000000,
 	.max_downscale		=	4,
@@ -4348,6 +4362,8 @@ static const struct dispc_features omap36xx_dispc_feats = {
 	.mgr_height_start	=	26,
 	.mgr_width_max		=	2048,
 	.mgr_height_max		=	2048,
+	.ovl_width_max		=	2048,
+	.ovl_height_max		=	2048,
 	.max_lcd_pclk		=	173000000,
 	.max_tv_pclk		=	59000000,
 	.max_downscale		=	4,
@@ -4383,6 +4399,8 @@ static const struct dispc_features am43xx_dispc_feats = {
 	.mgr_height_start	=	26,
 	.mgr_width_max		=	2048,
 	.mgr_height_max		=	2048,
+	.ovl_width_max		=	2048,
+	.ovl_height_max		=	2048,
 	.max_lcd_pclk		=	173000000,
 	.max_tv_pclk		=	59000000,
 	.max_downscale		=	4,
@@ -4418,6 +4436,8 @@ static const struct dispc_features omap44xx_dispc_feats = {
 	.mgr_height_start	=	26,
 	.mgr_width_max		=	2048,
 	.mgr_height_max		=	2048,
+	.ovl_width_max		=	2048,
+	.ovl_height_max		=	2048,
 	.max_lcd_pclk		=	170000000,
 	.max_tv_pclk		=	185625000,
 	.max_downscale		=	4,
@@ -4457,6 +4477,8 @@ static const struct dispc_features omap54xx_dispc_feats = {
 	.mgr_height_start	=	27,
 	.mgr_width_max		=	4096,
 	.mgr_height_max		=	4096,
+	.ovl_width_max		=	2048,
+	.ovl_height_max		=	4096,
 	.max_lcd_pclk		=	170000000,
 	.max_tv_pclk		=	186000000,
 	.max_downscale		=	4,
diff --git a/drivers/gpu/drm/omapdrm/dss/dss.h b/drivers/gpu/drm/omapdrm/dss/dss.h
index a547527bb2f3..14c39f7c3988 100644
--- a/drivers/gpu/drm/omapdrm/dss/dss.h
+++ b/drivers/gpu/drm/omapdrm/dss/dss.h
@@ -397,6 +397,8 @@ int dispc_get_num_mgrs(struct dispc_device *dispc);
 const u32 *dispc_ovl_get_color_modes(struct dispc_device *dispc,
 					    enum omap_plane_id plane);
 
+void dispc_ovl_get_max_size(struct dispc_device *dispc, u16 *width, u16 *height);
+
 u32 dispc_read_irqstatus(struct dispc_device *dispc);
 void dispc_clear_irqstatus(struct dispc_device *dispc, u32 mask);
 void dispc_write_irqenable(struct dispc_device *dispc, u32 mask);
diff --git a/drivers/gpu/drm/omapdrm/omap_plane.c b/drivers/gpu/drm/omapdrm/omap_plane.c
index c3de4f339387..846698c21a4a 100644
--- a/drivers/gpu/drm/omapdrm/omap_plane.c
+++ b/drivers/gpu/drm/omapdrm/omap_plane.c
@@ -111,12 +111,19 @@ static int omap_plane_atomic_check(struct drm_plane *plane,
 {
 	struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(state,
 										 plane);
+	struct omap_drm_private *priv = plane->dev->dev_private;
 	struct drm_crtc_state *crtc_state;
+	u32 max_width, max_height;
+	u16 width, height;
 	int ret;
 
 	if (!new_plane_state->fb)
 		return 0;
 
+	dispc_ovl_get_max_size(priv->dispc, &width, &height);
+	max_width = width << 16;
+	max_height = height << 16;
+
 	/* crtc should only be NULL when disabling (i.e., !new_plane_state->fb) */
 	if (WARN_ON(!new_plane_state->crtc))
 		return 0;
@@ -151,6 +158,13 @@ static int omap_plane_atomic_check(struct drm_plane *plane,
 	if (new_plane_state->crtc_y + new_plane_state->crtc_h > crtc_state->adjusted_mode.vdisplay)
 		return -EINVAL;
 
+	/* Make sure dimensions are within bounds. */
+	if (new_plane_state->src_h > max_height || new_plane_state->crtc_h > height)
+		return -EINVAL;
+
+	if (new_plane_state->src_w > max_width || new_plane_state->crtc_w > width)
+		return -EINVAL;
+
 	if (new_plane_state->rotation != DRM_MODE_ROTATE_0 &&
 	    !omap_framebuffer_supports_rotation(new_plane_state->fb))
 		return -EINVAL;
-- 
2.25.1

[PATCH v6 3/9] drm/omap: Add ovl checking funcs to dispc_ops

[Neil Armstrong]

From: Benoit Parrot <bparrot@ti.com>

In order to be able to dynamically assign overlays to planes we need to
be able to asses the overlay capabilities.

Add a helper function to be able to retrieve the supported capabilities
of an overlay.

And export the function to check if a fourcc is supported on a given
overlay.

Signed-off-by: Benoit Parrot <bparrot@ti.com>
Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
---
 drivers/gpu/drm/omapdrm/dss/dispc.c | 9 +++++++--
 drivers/gpu/drm/omapdrm/dss/dss.h   | 3 +++
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/dss/dispc.c b/drivers/gpu/drm/omapdrm/dss/dispc.c
index 5ac90ba6b76d..dd36db75b4b4 100644
--- a/drivers/gpu/drm/omapdrm/dss/dispc.c
+++ b/drivers/gpu/drm/omapdrm/dss/dispc.c
@@ -1281,8 +1281,8 @@ static u32 dispc_ovl_get_burst_size(struct dispc_device *dispc,
 	return dispc->feat->burst_size_unit * 8;
 }
 
-static bool dispc_ovl_color_mode_supported(struct dispc_device *dispc,
-					   enum omap_plane_id plane, u32 fourcc)
+bool dispc_ovl_color_mode_supported(struct dispc_device *dispc,
+				    enum omap_plane_id plane, u32 fourcc)
 {
 	const u32 *modes;
 	unsigned int i;
@@ -2489,6 +2489,11 @@ static int dispc_ovl_calc_scaling_44xx(struct dispc_device *dispc,
 	return 0;
 }
 
+enum omap_overlay_caps dispc_ovl_get_caps(struct dispc_device *dispc, enum omap_plane_id plane)
+{
+	return dispc->feat->overlay_caps[plane];
+}
+
 #define DIV_FRAC(dividend, divisor) \
 	((dividend) * 100 / (divisor) - ((dividend) / (divisor) * 100))
 
diff --git a/drivers/gpu/drm/omapdrm/dss/dss.h b/drivers/gpu/drm/omapdrm/dss/dss.h
index 14c39f7c3988..4ff02fbc0e71 100644
--- a/drivers/gpu/drm/omapdrm/dss/dss.h
+++ b/drivers/gpu/drm/omapdrm/dss/dss.h
@@ -398,6 +398,9 @@ const u32 *dispc_ovl_get_color_modes(struct dispc_device *dispc,
 					    enum omap_plane_id plane);
 
 void dispc_ovl_get_max_size(struct dispc_device *dispc, u16 *width, u16 *height);
+bool dispc_ovl_color_mode_supported(struct dispc_device *dispc,
+				    enum omap_plane_id plane, u32 fourcc);
+enum omap_overlay_caps dispc_ovl_get_caps(struct dispc_device *dispc, enum omap_plane_id plane);
 
 u32 dispc_read_irqstatus(struct dispc_device *dispc);
 void dispc_clear_irqstatus(struct dispc_device *dispc, u32 mask);
-- 
2.25.1

[PATCH v6 4/9] drm/omap: introduce omap_hw_overlay

[Neil Armstrong]

From: Benoit Parrot <bparrot@ti.com>

Split out the hardware overlay specifics from omap_plane.
To start, the hw overlays are statically assigned to planes.

The goal is to eventually assign hw overlays dynamically to planes
during plane->atomic_check() based on requested caps (scaling, YUV,
etc). And then perform hw overlay re-assignment if required.

Signed-off-by: Benoit Parrot <bparrot@ti.com>
Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
---
 drivers/gpu/drm/omapdrm/Makefile       |  1 +
 drivers/gpu/drm/omapdrm/omap_drv.c     |  9 ++-
 drivers/gpu/drm/omapdrm/omap_drv.h     |  4 ++
 drivers/gpu/drm/omapdrm/omap_overlay.c | 84 ++++++++++++++++++++++++++
 drivers/gpu/drm/omapdrm/omap_overlay.h | 30 +++++++++
 drivers/gpu/drm/omapdrm/omap_plane.c   | 54 +++++++----------
 6 files changed, 148 insertions(+), 34 deletions(-)
 create mode 100644 drivers/gpu/drm/omapdrm/omap_overlay.c
 create mode 100644 drivers/gpu/drm/omapdrm/omap_overlay.h

diff --git a/drivers/gpu/drm/omapdrm/Makefile b/drivers/gpu/drm/omapdrm/Makefile
index 21e8277ff88f..710b4e0abcf0 100644
--- a/drivers/gpu/drm/omapdrm/Makefile
+++ b/drivers/gpu/drm/omapdrm/Makefile
@@ -9,6 +9,7 @@ omapdrm-y := omap_drv.o \
 	omap_debugfs.o \
 	omap_crtc.o \
 	omap_plane.o \
+	omap_overlay.o \
 	omap_encoder.o \
 	omap_fb.o \
 	omap_gem.o \
diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c
index f86e20578143..b994014b22e8 100644
--- a/drivers/gpu/drm/omapdrm/omap_drv.c
+++ b/drivers/gpu/drm/omapdrm/omap_drv.c
@@ -583,10 +583,14 @@ static int omapdrm_init(struct omap_drm_private *priv, struct device *dev)
 
 	omap_gem_init(ddev);
 
+	ret = omap_hwoverlays_init(priv);
+	if (ret)
+		goto err_gem_deinit;
+
 	ret = omap_modeset_init(ddev);
 	if (ret) {
 		dev_err(priv->dev, "omap_modeset_init failed: ret=%d\n", ret);
-		goto err_gem_deinit;
+		goto err_free_overlays;
 	}
 
 	/* Initialize vblank handling, start with all CRTCs disabled. */
@@ -618,6 +622,8 @@ static int omapdrm_init(struct omap_drm_private *priv, struct device *dev)
 	omap_fbdev_fini(ddev);
 err_cleanup_modeset:
 	omap_modeset_fini(ddev);
+err_free_overlays:
+	omap_hwoverlays_destroy(priv);
 err_gem_deinit:
 	omap_gem_deinit(ddev);
 	destroy_workqueue(priv->wq);
@@ -642,6 +648,7 @@ static void omapdrm_cleanup(struct omap_drm_private *priv)
 	drm_atomic_helper_shutdown(ddev);
 
 	omap_modeset_fini(ddev);
+	omap_hwoverlays_destroy(priv);
 	omap_gem_deinit(ddev);
 
 	destroy_workqueue(priv->wq);
diff --git a/drivers/gpu/drm/omapdrm/omap_drv.h b/drivers/gpu/drm/omapdrm/omap_drv.h
index 591d4c273f02..b4d9c2062723 100644
--- a/drivers/gpu/drm/omapdrm/omap_drv.h
+++ b/drivers/gpu/drm/omapdrm/omap_drv.h
@@ -24,6 +24,7 @@
 #include "omap_gem.h"
 #include "omap_irq.h"
 #include "omap_plane.h"
+#include "omap_overlay.h"
 
 #define DBG(fmt, ...) DRM_DEBUG_DRIVER(fmt"\n", ##__VA_ARGS__)
 #define VERB(fmt, ...) if (0) DRM_DEBUG_DRIVER(fmt, ##__VA_ARGS__) /* verbose debug */
@@ -57,6 +58,9 @@ struct omap_drm_private {
 	unsigned int num_planes;
 	struct drm_plane *planes[8];
 
+	unsigned int num_ovls;
+	struct omap_hw_overlay *overlays[8];
+
 	struct drm_fb_helper *fbdev;
 
 	struct workqueue_struct *wq;
diff --git a/drivers/gpu/drm/omapdrm/omap_overlay.c b/drivers/gpu/drm/omapdrm/omap_overlay.c
new file mode 100644
index 000000000000..de67b39529bd
--- /dev/null
+++ b/drivers/gpu/drm/omapdrm/omap_overlay.c
@@ -0,0 +1,84 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2018 Texas Instruments Incorporated -  http://www.ti.com/
+ * Author: Benoit Parrot <bparrot@ti.com>
+ */
+
+#include <drm/drm_atomic.h>
+#include <drm/drm_atomic_helper.h>
+#include <drm/drm_plane_helper.h>
+
+#include "omap_dmm_tiler.h"
+#include "omap_drv.h"
+
+/*
+ * overlay funcs
+ */
+static const char * const overlay_id_to_name[] = {
+	[OMAP_DSS_GFX] = "gfx",
+	[OMAP_DSS_VIDEO1] = "vid1",
+	[OMAP_DSS_VIDEO2] = "vid2",
+	[OMAP_DSS_VIDEO3] = "vid3",
+};
+
+static void omap_overlay_destroy(struct omap_hw_overlay *overlay)
+{
+	kfree(overlay);
+}
+
+static struct omap_hw_overlay *omap_overlay_init(enum omap_plane_id overlay_id,
+						 enum omap_overlay_caps caps)
+{
+	struct omap_hw_overlay *overlay;
+
+	overlay = kzalloc(sizeof(*overlay), GFP_KERNEL);
+	if (!overlay)
+		return ERR_PTR(-ENOMEM);
+
+	overlay->name = overlay_id_to_name[overlay_id];
+	overlay->id = overlay_id;
+	overlay->caps = caps;
+
+	return overlay;
+}
+
+int omap_hwoverlays_init(struct omap_drm_private *priv)
+{
+	static const enum omap_plane_id hw_plane_ids[] = {
+			OMAP_DSS_GFX, OMAP_DSS_VIDEO1,
+			OMAP_DSS_VIDEO2, OMAP_DSS_VIDEO3,
+	};
+	u32 num_overlays = dispc_get_num_ovls(priv->dispc);
+	enum omap_overlay_caps caps;
+	int i, ret;
+
+	for (i = 0; i < num_overlays; i++) {
+		struct omap_hw_overlay *overlay;
+
+		caps = dispc_ovl_get_caps(priv->dispc, hw_plane_ids[i]);
+		overlay = omap_overlay_init(hw_plane_ids[i], caps);
+		if (IS_ERR(overlay)) {
+			ret = PTR_ERR(overlay);
+			dev_err(priv->dev, "failed to construct overlay for %s (%d)\n",
+				overlay_id_to_name[i], ret);
+			omap_hwoverlays_destroy(priv);
+			return ret;
+		}
+		overlay->idx = priv->num_ovls;
+		priv->overlays[priv->num_ovls++] = overlay;
+	}
+
+	return 0;
+}
+
+void omap_hwoverlays_destroy(struct omap_drm_private *priv)
+{
+	int i;
+
+	for (i = 0; i < priv->num_ovls; i++) {
+		omap_overlay_destroy(priv->overlays[i]);
+		priv->overlays[i] = NULL;
+	}
+
+	priv->num_ovls = 0;
+}
diff --git a/drivers/gpu/drm/omapdrm/omap_overlay.h b/drivers/gpu/drm/omapdrm/omap_overlay.h
new file mode 100644
index 000000000000..51d26aebf77d
--- /dev/null
+++ b/drivers/gpu/drm/omapdrm/omap_overlay.h
@@ -0,0 +1,30 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (C) 2018 Texas Instruments Incorporated -  http://www.ti.com/
+ * Author: Benoit Parrot <bparrot@ti.com>
+ */
+
+#ifndef __OMAPDRM_OVERLAY_H__
+#define __OMAPDRM_OVERLAY_H__
+
+#include <linux/types.h>
+
+enum drm_plane_type;
+
+struct drm_device;
+struct drm_mode_object;
+struct drm_plane;
+
+/* Used to associate a HW overlay/plane to a plane */
+struct omap_hw_overlay {
+	unsigned int idx;
+
+	const char *name;
+	enum omap_plane_id id;
+
+	enum omap_overlay_caps caps;
+};
+
+int omap_hwoverlays_init(struct omap_drm_private *priv);
+void omap_hwoverlays_destroy(struct omap_drm_private *priv);
+#endif /* __OMAPDRM_OVERLAY_H__ */
diff --git a/drivers/gpu/drm/omapdrm/omap_plane.c b/drivers/gpu/drm/omapdrm/omap_plane.c
index 846698c21a4a..8cc2b899f20c 100644
--- a/drivers/gpu/drm/omapdrm/omap_plane.c
+++ b/drivers/gpu/drm/omapdrm/omap_plane.c
@@ -21,7 +21,8 @@
 struct omap_plane {
 	struct drm_plane base;
 	enum omap_plane_id id;
-	const char *name;
+
+	struct omap_hw_overlay *overlay;
 };
 
 static int omap_plane_prepare_fb(struct drm_plane *plane,
@@ -49,10 +50,11 @@ static void omap_plane_atomic_update(struct drm_plane *plane,
 	struct omap_plane *omap_plane = to_omap_plane(plane);
 	struct drm_plane_state *new_state = drm_atomic_get_new_plane_state(state,
 									   plane);
+	enum omap_plane_id ovl_id = omap_plane->overlay->id;
 	struct omap_overlay_info info;
 	int ret;
 
-	DBG("%s, crtc=%p fb=%p", omap_plane->name, new_state->crtc,
+	DBG("%s, crtc=%p fb=%p", plane->name, new_state->crtc,
 	    new_state->fb);
 
 	memset(&info, 0, sizeof(info));
@@ -77,17 +79,17 @@ static void omap_plane_atomic_update(struct drm_plane *plane,
 			&info.paddr, &info.p_uv_addr);
 
 	/* and finally, update omapdss: */
-	ret = dispc_ovl_setup(priv->dispc, omap_plane->id, &info,
+	ret = dispc_ovl_setup(priv->dispc, ovl_id, &info,
 			      omap_crtc_timings(new_state->crtc), false,
 			      omap_crtc_channel(new_state->crtc));
 	if (ret) {
 		dev_err(plane->dev->dev, "Failed to setup plane %s\n",
-			omap_plane->name);
-		dispc_ovl_enable(priv->dispc, omap_plane->id, false);
+			plane->name);
+		dispc_ovl_enable(priv->dispc, ovl_id, false);
 		return;
 	}
 
-	dispc_ovl_enable(priv->dispc, omap_plane->id, true);
+	dispc_ovl_enable(priv->dispc, ovl_id, true);
 }
 
 static void omap_plane_atomic_disable(struct drm_plane *plane,
@@ -97,11 +99,12 @@ static void omap_plane_atomic_disable(struct drm_plane *plane,
 									   plane);
 	struct omap_drm_private *priv = plane->dev->dev_private;
 	struct omap_plane *omap_plane = to_omap_plane(plane);
+	enum omap_plane_id ovl_id = omap_plane->overlay->id;
 
 	new_state->rotation = DRM_MODE_ROTATE_0;
 	new_state->zpos = plane->type == DRM_PLANE_TYPE_PRIMARY ? 0 : omap_plane->id;
 
-	dispc_ovl_enable(priv->dispc, omap_plane->id, false);
+	dispc_ovl_enable(priv->dispc, ovl_id, false);
 }
 
 #define FRAC_16_16(mult, div)    (((mult) << 16) / (div))
@@ -184,7 +187,7 @@ static void omap_plane_destroy(struct drm_plane *plane)
 {
 	struct omap_plane *omap_plane = to_omap_plane(plane);
 
-	DBG("%s", omap_plane->name);
+	DBG("%s", plane->name);
 
 	drm_plane_cleanup(plane);
 
@@ -290,20 +293,6 @@ static bool omap_plane_supports_yuv(struct drm_plane *plane)
 	return false;
 }
 
-static const char *plane_id_to_name[] = {
-	[OMAP_DSS_GFX] = "gfx",
-	[OMAP_DSS_VIDEO1] = "vid1",
-	[OMAP_DSS_VIDEO2] = "vid2",
-	[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,
-};
-
 /* initialize plane */
 struct drm_plane *omap_plane_init(struct drm_device *dev,
 		int idx, enum drm_plane_type type,
@@ -313,27 +302,26 @@ struct drm_plane *omap_plane_init(struct drm_device *dev,
 	unsigned int num_planes = dispc_get_num_ovls(priv->dispc);
 	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)))
+	if (WARN_ON(idx >= num_planes))
 		return ERR_PTR(-EINVAL);
 
-	id = plane_idx_to_id[idx];
-
-	DBG("%s: type=%d", plane_id_to_name[id], type);
-
 	omap_plane = kzalloc(sizeof(*omap_plane), GFP_KERNEL);
 	if (!omap_plane)
 		return ERR_PTR(-ENOMEM);
 
-	formats = dispc_ovl_get_color_modes(priv->dispc, id);
+	omap_plane->id = idx;
+	omap_plane->overlay = priv->overlays[idx];
+
+	DBG("%d: type=%d", omap_plane->id, type);
+	DBG("	crtc_mask: 0x%04x", possible_crtcs);
+
+	formats = dispc_ovl_get_color_modes(priv->dispc, omap_plane->overlay->id);
 	for (nformats = 0; formats[nformats]; ++nformats)
 		;
-	omap_plane->id = id;
-	omap_plane->name = plane_id_to_name[id];
 
 	plane = &omap_plane->base;
 
@@ -363,8 +351,8 @@ struct drm_plane *omap_plane_init(struct drm_device *dev,
 	return plane;
 
 error:
-	dev_err(dev->dev, "%s(): could not create plane: %s\n",
-		__func__, plane_id_to_name[id]);
+	dev_err(dev->dev, "%s(): could not create plane: %d\n",
+		__func__, omap_plane->id);
 
 	kfree(omap_plane);
 	return NULL;
-- 
2.25.1

[PATCH v6 5/9] drm/omap: omap_plane: subclass drm_plane_state

[Neil Armstrong]

From: Benoit Parrot <bparrot@ti.com>

In preparation to add omap plane state specific extensions we need to
subclass drm_plane_state and add the relevant helpers.

The addition of specific extension will be done separately.

Signed-off-by: Benoit Parrot <bparrot@ti.com>
Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
---
 drivers/gpu/drm/omapdrm/omap_plane.c | 36 +++++++++++++++++++++++++---
 1 file changed, 33 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_plane.c b/drivers/gpu/drm/omapdrm/omap_plane.c
index 8cc2b899f20c..72d5adbbadfa 100644
--- a/drivers/gpu/drm/omapdrm/omap_plane.c
+++ b/drivers/gpu/drm/omapdrm/omap_plane.c
@@ -16,6 +16,13 @@
  * plane funcs
  */
 
+#define to_omap_plane_state(x) container_of(x, struct omap_plane_state, base)
+
+struct omap_plane_state {
+	/* Must be first. */
+	struct drm_plane_state base;
+};
+
 #define to_omap_plane(x) container_of(x, struct omap_plane, base)
 
 struct omap_plane {
@@ -221,11 +228,17 @@ void omap_plane_install_properties(struct drm_plane *plane,
 static void omap_plane_reset(struct drm_plane *plane)
 {
 	struct omap_plane *omap_plane = to_omap_plane(plane);
+	struct omap_plane_state *omap_state;
 
-	drm_atomic_helper_plane_reset(plane);
-	if (!plane->state)
+	if (plane->state)
+		drm_atomic_helper_plane_destroy_state(plane, plane->state);
+
+	omap_state = kzalloc(sizeof(*omap_state), GFP_KERNEL);
+	if (!omap_state)
 		return;
 
+	__drm_atomic_helper_plane_reset(plane, &omap_state->base);
+
 	/*
 	 * Set the zpos default depending on whether we are a primary or overlay
 	 * plane.
@@ -236,6 +249,23 @@ static void omap_plane_reset(struct drm_plane *plane)
 	plane->state->color_range = DRM_COLOR_YCBCR_FULL_RANGE;
 }
 
+static struct drm_plane_state *
+omap_plane_atomic_duplicate_state(struct drm_plane *plane)
+{
+	struct omap_plane_state *state;
+
+	if (WARN_ON(!plane->state))
+		return NULL;
+
+	state = kmalloc(sizeof(*state), GFP_KERNEL);
+	if (!state)
+		return NULL;
+
+	__drm_atomic_helper_plane_duplicate_state(plane, &state->base);
+
+	return &state->base;
+}
+
 static int omap_plane_atomic_set_property(struct drm_plane *plane,
 					  struct drm_plane_state *state,
 					  struct drm_property *property,
@@ -271,7 +301,7 @@ static const struct drm_plane_funcs omap_plane_funcs = {
 	.disable_plane = drm_atomic_helper_disable_plane,
 	.reset = omap_plane_reset,
 	.destroy = omap_plane_destroy,
-	.atomic_duplicate_state = drm_atomic_helper_plane_duplicate_state,
+	.atomic_duplicate_state = omap_plane_atomic_duplicate_state,
 	.atomic_destroy_state = drm_atomic_helper_plane_destroy_state,
 	.atomic_set_property = omap_plane_atomic_set_property,
 	.atomic_get_property = omap_plane_atomic_get_property,
-- 
2.25.1

[PATCH v6 6/9] drm/omap: Add global state as a private atomic object

[Neil Armstrong]

From: Benoit Parrot <bparrot@ti.com>

Global shared resources (like hw overlays) for omapdrm are implemented
as a part of atomic state using the drm_private_obj infrastructure
available in the atomic core.

omap_global_state is introduced as a drm atomic private object. The two
funcs omap_get_global_state() and omap_get_existing_global_state() are
the two variants that will be used to access omap_global_state.

drm_mode_config_init() needs to be called earlier because it
creates/initializes the private_obj link list maintained by the atomic
framework. The private_obj link list has to exist prior to calling
drm_atomic_private_obj_init(). Similarly the cleanup handler are
reordered appropriately.

Signed-off-by: Benoit Parrot <bparrot@ti.com>
Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
---
 drivers/gpu/drm/omapdrm/omap_drv.c | 91 +++++++++++++++++++++++++++++-
 drivers/gpu/drm/omapdrm/omap_drv.h | 17 ++++++
 2 files changed, 105 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c
index b994014b22e8..c7912374d393 100644
--- a/drivers/gpu/drm/omapdrm/omap_drv.c
+++ b/drivers/gpu/drm/omapdrm/omap_drv.c
@@ -128,6 +128,82 @@ static const struct drm_mode_config_funcs omap_mode_config_funcs = {
 	.atomic_commit = drm_atomic_helper_commit,
 };
 
+/* Global/shared object state funcs */
+
+/*
+ * This is a helper that returns the private state currently in operation.
+ * Note that this would return the "old_state" if called in the atomic check
+ * path, and the "new_state" after the atomic swap has been done.
+ */
+struct omap_global_state *
+omap_get_existing_global_state(struct omap_drm_private *priv)
+{
+	return to_omap_global_state(priv->glob_obj.state);
+}
+
+/*
+ * This acquires the modeset lock set aside for global state, creates
+ * a new duplicated private object state.
+ */
+struct omap_global_state *__must_check
+omap_get_global_state(struct drm_atomic_state *s)
+{
+	struct omap_drm_private *priv = s->dev->dev_private;
+	struct drm_private_state *priv_state;
+
+	priv_state = drm_atomic_get_private_obj_state(s, &priv->glob_obj);
+	if (IS_ERR(priv_state))
+		return ERR_CAST(priv_state);
+
+	return to_omap_global_state(priv_state);
+}
+
+static struct drm_private_state *
+omap_global_duplicate_state(struct drm_private_obj *obj)
+{
+	struct omap_global_state *state;
+
+	state = kmemdup(obj->state, sizeof(*state), GFP_KERNEL);
+	if (!state)
+		return NULL;
+
+	__drm_atomic_helper_private_obj_duplicate_state(obj, &state->base);
+
+	return &state->base;
+}
+
+static void omap_global_destroy_state(struct drm_private_obj *obj,
+				      struct drm_private_state *state)
+{
+	struct omap_global_state *omap_state = to_omap_global_state(state);
+
+	kfree(omap_state);
+}
+
+static const struct drm_private_state_funcs omap_global_state_funcs = {
+	.atomic_duplicate_state = omap_global_duplicate_state,
+	.atomic_destroy_state = omap_global_destroy_state,
+};
+
+static int omap_global_obj_init(struct drm_device *dev)
+{
+	struct omap_drm_private *priv = dev->dev_private;
+	struct omap_global_state *state;
+
+	state = kzalloc(sizeof(*state), GFP_KERNEL);
+	if (!state)
+		return -ENOMEM;
+
+	drm_atomic_private_obj_init(dev, &priv->glob_obj, &state->base,
+				    &omap_global_state_funcs);
+	return 0;
+}
+
+static void omap_global_obj_fini(struct omap_drm_private *priv)
+{
+	drm_atomic_private_obj_fini(&priv->glob_obj);
+}
+
 static void omap_disconnect_pipelines(struct drm_device *ddev)
 {
 	struct omap_drm_private *priv = ddev->dev_private;
@@ -231,8 +307,6 @@ static int omap_modeset_init(struct drm_device *dev)
 	if (!omapdss_stack_is_ready())
 		return -EPROBE_DEFER;
 
-	drm_mode_config_init(dev);
-
 	ret = omap_modeset_init_properties(dev);
 	if (ret < 0)
 		return ret;
@@ -583,10 +657,16 @@ static int omapdrm_init(struct omap_drm_private *priv, struct device *dev)
 
 	omap_gem_init(ddev);
 
-	ret = omap_hwoverlays_init(priv);
+	drm_mode_config_init(ddev);
+
+	ret = omap_global_obj_init(ddev);
 	if (ret)
 		goto err_gem_deinit;
 
+	ret = omap_hwoverlays_init(priv);
+	if (ret)
+		goto err_free_priv_obj;
+
 	ret = omap_modeset_init(ddev);
 	if (ret) {
 		dev_err(priv->dev, "omap_modeset_init failed: ret=%d\n", ret);
@@ -624,7 +704,10 @@ static int omapdrm_init(struct omap_drm_private *priv, struct device *dev)
 	omap_modeset_fini(ddev);
 err_free_overlays:
 	omap_hwoverlays_destroy(priv);
+err_free_priv_obj:
+	omap_global_obj_fini(priv);
 err_gem_deinit:
+	drm_mode_config_cleanup(ddev);
 	omap_gem_deinit(ddev);
 	destroy_workqueue(priv->wq);
 	omap_disconnect_pipelines(ddev);
@@ -649,6 +732,8 @@ static void omapdrm_cleanup(struct omap_drm_private *priv)
 
 	omap_modeset_fini(ddev);
 	omap_hwoverlays_destroy(priv);
+	omap_global_obj_fini(priv);
+	drm_mode_config_cleanup(ddev);
 	omap_gem_deinit(ddev);
 
 	destroy_workqueue(priv->wq);
diff --git a/drivers/gpu/drm/omapdrm/omap_drv.h b/drivers/gpu/drm/omapdrm/omap_drv.h
index b4d9c2062723..a3079aad7ae3 100644
--- a/drivers/gpu/drm/omapdrm/omap_drv.h
+++ b/drivers/gpu/drm/omapdrm/omap_drv.h
@@ -14,6 +14,7 @@
 #include "dss/omapdss.h"
 #include "dss/dss.h"
 
+#include <drm/drm_atomic.h>
 #include <drm/drm_gem.h>
 #include <drm/omap_drm.h>
 
@@ -41,6 +42,16 @@ struct omap_drm_pipeline {
 	unsigned int alias_id;
 };
 
+/*
+ * Global private object state for tracking resources that are shared across
+ * multiple kms objects (planes/crtcs/etc).
+ */
+#define to_omap_global_state(x) container_of(x, struct omap_global_state, base)
+
+struct omap_global_state {
+	struct drm_private_state base;
+};
+
 struct omap_drm_private {
 	struct drm_device *ddev;
 	struct device *dev;
@@ -61,6 +72,8 @@ struct omap_drm_private {
 	unsigned int num_ovls;
 	struct omap_hw_overlay *overlays[8];
 
+	struct drm_private_obj glob_obj;
+
 	struct drm_fb_helper *fbdev;
 
 	struct workqueue_struct *wq;
@@ -89,4 +102,8 @@ struct omap_drm_private {
 
 void omap_debugfs_init(struct drm_minor *minor);
 
+struct omap_global_state * __must_check omap_get_global_state(struct drm_atomic_state *s);
+
+struct omap_global_state *omap_get_existing_global_state(struct omap_drm_private *priv);
+
 #endif /* __OMAPDRM_DRV_H__ */
-- 
2.25.1

[PATCH v6 7/9] drm/omap: dynamically assign hw overlays to planes

[Neil Armstrong]

From: Benoit Parrot <bparrot@ti.com>

(re)assign the hw overlays to planes based on required caps, and to
handle situations where we could not modify an in-use plane.

This means all planes advertise the superset of formats and properties.
Userspace must (as always) use atomic TEST_ONLY step for atomic updates,
as not all planes may be available for use on every frame.

The mapping of hwoverlays to plane is stored in omap_global_state, so
that state updates are atomically committed in the same way that
plane/etc state updates are managed.  This is needed because the
omap_plane_state keeps a pointer to the hwoverlay, and we don't want
global state to become out of sync with the plane state if an atomic
update fails, we hit deadlock/ backoff scenario, etc.  The use of
global_state_lock keeps multiple parallel updates which both re-assign
hwoverlays properly serialized.

Signed-off-by: Benoit Parrot <bparrot@ti.com>
Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
---
 drivers/gpu/drm/omapdrm/omap_drv.h     |   3 +
 drivers/gpu/drm/omapdrm/omap_overlay.c | 109 +++++++++++++++++++++
 drivers/gpu/drm/omapdrm/omap_overlay.h |   4 +
 drivers/gpu/drm/omapdrm/omap_plane.c   | 127 ++++++++++++++++++++-----
 4 files changed, 220 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_drv.h b/drivers/gpu/drm/omapdrm/omap_drv.h
index a3079aad7ae3..825960fd3ea9 100644
--- a/drivers/gpu/drm/omapdrm/omap_drv.h
+++ b/drivers/gpu/drm/omapdrm/omap_drv.h
@@ -50,6 +50,9 @@ struct omap_drm_pipeline {
 
 struct omap_global_state {
 	struct drm_private_state base;
+
+	/* global atomic state of assignment between overlays and planes */
+	struct drm_plane *hwoverlay_to_plane[8];
 };
 
 struct omap_drm_private {
diff --git a/drivers/gpu/drm/omapdrm/omap_overlay.c b/drivers/gpu/drm/omapdrm/omap_overlay.c
index de67b39529bd..afb2b44fdf86 100644
--- a/drivers/gpu/drm/omapdrm/omap_overlay.c
+++ b/drivers/gpu/drm/omapdrm/omap_overlay.c
@@ -21,6 +21,115 @@ static const char * const overlay_id_to_name[] = {
 	[OMAP_DSS_VIDEO3] = "vid3",
 };
 
+/*
+ * Find a free overlay with the required caps and supported fourcc
+ */
+static struct omap_hw_overlay *
+omap_plane_find_free_overlay(struct drm_device *dev, struct drm_plane *hwoverlay_to_plane[],
+			     u32 caps, u32 fourcc)
+{
+	struct omap_drm_private *priv = dev->dev_private;
+	int i;
+
+	DBG("caps: %x fourcc: %x", caps, fourcc);
+
+	for (i = 0; i < priv->num_ovls; i++) {
+		struct omap_hw_overlay *cur = priv->overlays[i];
+
+		DBG("%d: id: %d cur->caps: %x",
+		    cur->idx, cur->id, cur->caps);
+
+		/* skip if already in-use */
+		if (hwoverlay_to_plane[cur->idx])
+			continue;
+
+		/* skip if doesn't support some required caps: */
+		if (caps & ~cur->caps)
+			continue;
+
+		/* check supported format */
+		if (!dispc_ovl_color_mode_supported(priv->dispc,
+						    cur->id, fourcc))
+			continue;
+
+		return cur;
+	}
+
+	DBG("no match");
+	return NULL;
+}
+
+/*
+ * Assign a new overlay to a plane with the required caps and supported fourcc
+ * If a plane need a new overlay, the previous one should have been released
+ * with omap_overlay_release()
+ * This should be called from the plane atomic_check() in order to prepare the
+ * next global overlay_map to be enabled when atomic transaction is valid.
+ */
+int omap_overlay_assign(struct drm_atomic_state *s, struct drm_plane *plane,
+			u32 caps, u32 fourcc, struct omap_hw_overlay **overlay)
+{
+	/* Get the global state of the current atomic transaction */
+	struct omap_global_state *state = omap_get_global_state(s);
+	struct drm_plane **overlay_map = state->hwoverlay_to_plane;
+	struct omap_hw_overlay *ovl;
+
+	ovl = omap_plane_find_free_overlay(s->dev, overlay_map, caps, fourcc);
+	if (!ovl)
+		return -ENOMEM;
+
+	overlay_map[ovl->idx] = plane;
+	*overlay = ovl;
+
+	DBG("%s: assign to plane %s caps %x", ovl->name, plane->name, caps);
+
+	return 0;
+}
+
+/*
+ * Release an overlay from a plane if the plane gets not visible or the plane
+ * need a new overlay if overlay caps changes.
+ * This should be called from the plane atomic_check() in order to prepare the
+ * next global overlay_map to be enabled when atomic transaction is valid.
+ */
+void omap_overlay_release(struct drm_atomic_state *s, struct omap_hw_overlay *overlay)
+{
+	/* Get the global state of the current atomic transaction */
+	struct omap_global_state *state = omap_get_global_state(s);
+	struct drm_plane **overlay_map = state->hwoverlay_to_plane;
+
+	if (!overlay)
+		return;
+
+	if (WARN_ON(!overlay_map[overlay->idx]))
+		return;
+
+	DBG("%s: release from plane %s", overlay->name, overlay_map[overlay->idx]->name);
+
+	overlay_map[overlay->idx] = NULL;
+}
+
+/*
+ * Update an overlay state that was attached to a plane before the current atomic state.
+ * This should be called from the plane atomic_update() or atomic_disable(),
+ * where an overlay association to a plane could have changed between the old and current
+ * atomic state.
+ */
+void omap_overlay_update_state(struct omap_drm_private *priv,
+			       struct omap_hw_overlay *overlay)
+{
+	struct omap_global_state *state = omap_get_existing_global_state(priv);
+	struct drm_plane **overlay_map = state->hwoverlay_to_plane;
+
+	/* Check if this overlay is not used anymore, then disable it */
+	if (!overlay_map[overlay->idx]) {
+		DBG("%s: disabled", overlay->name);
+
+		/* disable the overlay */
+		dispc_ovl_enable(priv->dispc, overlay->id, false);
+	}
+}
+
 static void omap_overlay_destroy(struct omap_hw_overlay *overlay)
 {
 	kfree(overlay);
diff --git a/drivers/gpu/drm/omapdrm/omap_overlay.h b/drivers/gpu/drm/omapdrm/omap_overlay.h
index 51d26aebf77d..38ca55e1e2d7 100644
--- a/drivers/gpu/drm/omapdrm/omap_overlay.h
+++ b/drivers/gpu/drm/omapdrm/omap_overlay.h
@@ -27,4 +27,8 @@ struct omap_hw_overlay {
 
 int omap_hwoverlays_init(struct omap_drm_private *priv);
 void omap_hwoverlays_destroy(struct omap_drm_private *priv);
+int omap_overlay_assign(struct drm_atomic_state *s, struct drm_plane *plane,
+			u32 caps, u32 fourcc, struct omap_hw_overlay **overlay);
+void omap_overlay_release(struct drm_atomic_state *s, struct omap_hw_overlay *overlay);
+void omap_overlay_update_state(struct omap_drm_private *priv, struct omap_hw_overlay *overlay);
 #endif /* __OMAPDRM_OVERLAY_H__ */
diff --git a/drivers/gpu/drm/omapdrm/omap_plane.c b/drivers/gpu/drm/omapdrm/omap_plane.c
index 72d5adbbadfa..ce5ed45401fb 100644
--- a/drivers/gpu/drm/omapdrm/omap_plane.c
+++ b/drivers/gpu/drm/omapdrm/omap_plane.c
@@ -8,6 +8,7 @@
 #include <drm/drm_atomic_helper.h>
 #include <drm/drm_gem_atomic_helper.h>
 #include <drm/drm_plane_helper.h>
+#include <drm/drm_fourcc.h>
 
 #include "omap_dmm_tiler.h"
 #include "omap_drv.h"
@@ -21,6 +22,8 @@
 struct omap_plane_state {
 	/* Must be first. */
 	struct drm_plane_state base;
+
+	struct omap_hw_overlay *overlay;
 };
 
 #define to_omap_plane(x) container_of(x, struct omap_plane, base)
@@ -28,8 +31,6 @@ struct omap_plane_state {
 struct omap_plane {
 	struct drm_plane base;
 	enum omap_plane_id id;
-
-	struct omap_hw_overlay *overlay;
 };
 
 static int omap_plane_prepare_fb(struct drm_plane *plane,
@@ -54,13 +55,29 @@ static void omap_plane_atomic_update(struct drm_plane *plane,
 				     struct drm_atomic_state *state)
 {
 	struct omap_drm_private *priv = plane->dev->dev_private;
-	struct omap_plane *omap_plane = to_omap_plane(plane);
 	struct drm_plane_state *new_state = drm_atomic_get_new_plane_state(state,
 									   plane);
-	enum omap_plane_id ovl_id = omap_plane->overlay->id;
+	struct drm_plane_state *old_state = drm_atomic_get_old_plane_state(state,
+									   plane);
+	struct omap_plane_state *new_omap_state;
+	struct omap_plane_state *old_omap_state;
 	struct omap_overlay_info info;
+	enum omap_plane_id ovl_id;
 	int ret;
 
+	new_omap_state = to_omap_plane_state(new_state);
+	old_omap_state = to_omap_plane_state(old_state);
+
+	/* Cleanup previously held overlay if needed */
+	if (old_omap_state->overlay)
+		omap_overlay_update_state(priv, old_omap_state->overlay);
+
+	if (!new_omap_state->overlay) {
+		DBG("[PLANE:%d:%s] no overlay attached", plane->base.id, plane->name);
+		return;
+	}
+
+	ovl_id = new_omap_state->overlay->id;
 	DBG("%s, crtc=%p fb=%p", plane->name, new_state->crtc,
 	    new_state->fb);
 
@@ -79,9 +96,9 @@ static void omap_plane_atomic_update(struct drm_plane *plane,
 	/* update scanout: */
 	omap_framebuffer_update_scanout(new_state->fb, new_state, &info);
 
-	DBG("%dx%d -> %dx%d (%d)", info.width, info.height,
-			info.out_width, info.out_height,
-			info.screen_width);
+	DBG("%s: %dx%d -> %dx%d (%d)",
+			new_omap_state->overlay->name, 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);
 
@@ -102,16 +119,26 @@ static void omap_plane_atomic_update(struct drm_plane *plane,
 static void omap_plane_atomic_disable(struct drm_plane *plane,
 				      struct drm_atomic_state *state)
 {
-	struct drm_plane_state *new_state = drm_atomic_get_new_plane_state(state,
-									   plane);
 	struct omap_drm_private *priv = plane->dev->dev_private;
 	struct omap_plane *omap_plane = to_omap_plane(plane);
-	enum omap_plane_id ovl_id = omap_plane->overlay->id;
+	struct drm_plane_state *new_state = drm_atomic_get_new_plane_state(state,
+									   plane);
+	struct drm_plane_state *old_state = drm_atomic_get_old_plane_state(state,
+									   plane);
+	struct omap_plane_state *new_omap_state;
+	struct omap_plane_state *old_omap_state;
+
+	new_omap_state = to_omap_plane_state(new_state);
+	old_omap_state = to_omap_plane_state(old_state);
+
+	if (!old_omap_state->overlay)
+		return;
 
 	new_state->rotation = DRM_MODE_ROTATE_0;
 	new_state->zpos = plane->type == DRM_PLANE_TYPE_PRIMARY ? 0 : omap_plane->id;
 
-	dispc_ovl_enable(priv->dispc, ovl_id, false);
+	omap_overlay_update_state(priv, old_omap_state->overlay);
+	new_omap_state->overlay = NULL;
 }
 
 #define FRAC_16_16(mult, div)    (((mult) << 16) / (div))
@@ -121,32 +148,37 @@ static int omap_plane_atomic_check(struct drm_plane *plane,
 {
 	struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(state,
 										 plane);
+	struct drm_plane_state *old_plane_state = drm_atomic_get_old_plane_state(state,
+										 plane);
 	struct omap_drm_private *priv = plane->dev->dev_private;
+	struct omap_plane_state *omap_state = to_omap_plane_state(new_plane_state);
+	struct omap_global_state *omap_overlay_global_state;
 	struct drm_crtc_state *crtc_state;
+	bool new_hw_overlay = false;
 	u32 max_width, max_height;
+	struct drm_crtc *crtc;
 	u16 width, height;
+	u32 caps = 0;
+	u32 fourcc;
 	int ret;
 
-	if (!new_plane_state->fb)
-		return 0;
+	omap_overlay_global_state = omap_get_global_state(state);
+	if (IS_ERR(omap_overlay_global_state))
+		return PTR_ERR(omap_overlay_global_state);
 
 	dispc_ovl_get_max_size(priv->dispc, &width, &height);
 	max_width = width << 16;
 	max_height = height << 16;
 
-	/* crtc should only be NULL when disabling (i.e., !new_plane_state->fb) */
-	if (WARN_ON(!new_plane_state->crtc))
+	crtc = new_plane_state->crtc ? new_plane_state->crtc : plane->state->crtc;
+	if (!crtc)
 		return 0;
 
-	crtc_state = drm_atomic_get_existing_crtc_state(state,
-							new_plane_state->crtc);
+	crtc_state = drm_atomic_get_existing_crtc_state(state, crtc);
 	/* we should have a crtc state if the plane is attached to a crtc */
 	if (WARN_ON(!crtc_state))
 		return 0;
 
-	if (!crtc_state->enable)
-		return 0;
-
 	/*
 	 * Note: these are just sanity checks to filter out totally bad scaling
 	 * factors. The real limits must be calculated case by case, and
@@ -159,6 +191,15 @@ static int omap_plane_atomic_check(struct drm_plane *plane,
 	if (ret)
 		return ret;
 
+	DBG("%s: visible %d -> %d", plane->name,
+	    old_plane_state->visible, new_plane_state->visible);
+
+	if (!new_plane_state->visible) {
+		omap_overlay_release(state, omap_state->overlay);
+		omap_state->overlay = NULL;
+		return 0;
+	}
+
 	if (new_plane_state->crtc_x < 0 || new_plane_state->crtc_y < 0)
 		return -EINVAL;
 
@@ -179,6 +220,43 @@ static int omap_plane_atomic_check(struct drm_plane *plane,
 	    !omap_framebuffer_supports_rotation(new_plane_state->fb))
 		return -EINVAL;
 
+	if ((new_plane_state->src_w >> 16) != new_plane_state->crtc_w ||
+	    (new_plane_state->src_h >> 16) != new_plane_state->crtc_h)
+		caps |= OMAP_DSS_OVL_CAP_SCALE;
+
+	fourcc = new_plane_state->fb->format->format;
+
+	/*
+	 * (re)allocate hw overlay if we don't have one or
+	 * there is a caps mismatch
+	 */
+	if (!omap_state->overlay || (caps & ~omap_state->overlay->caps)) {
+		new_hw_overlay = true;
+	} else {
+		/* check supported format */
+		if (!dispc_ovl_color_mode_supported(priv->dispc, omap_state->overlay->id,
+						    fourcc))
+			new_hw_overlay = true;
+	}
+
+	if (new_hw_overlay) {
+		struct omap_hw_overlay *old_ovl = omap_state->overlay;
+		struct omap_hw_overlay *new_ovl = NULL;
+
+		omap_overlay_release(state, old_ovl);
+
+		ret = omap_overlay_assign(state, plane, caps, fourcc, &new_ovl);
+		if (ret) {
+			DBG("%s: failed to assign hw_overlay", plane->name);
+			omap_state->overlay = NULL;
+			return ret;
+		}
+
+		omap_state->overlay = new_ovl;
+	}
+
+	DBG("plane: %s overlay_id: %d", plane->name, omap_state->overlay->id);
+
 	return 0;
 }
 
@@ -252,17 +330,21 @@ static void omap_plane_reset(struct drm_plane *plane)
 static struct drm_plane_state *
 omap_plane_atomic_duplicate_state(struct drm_plane *plane)
 {
-	struct omap_plane_state *state;
+	struct omap_plane_state *state, *current_state;
 
 	if (WARN_ON(!plane->state))
 		return NULL;
 
+	current_state = to_omap_plane_state(plane->state);
+
 	state = kmalloc(sizeof(*state), GFP_KERNEL);
 	if (!state)
 		return NULL;
 
 	__drm_atomic_helper_plane_duplicate_state(plane, &state->base);
 
+	state->overlay = current_state->overlay;
+
 	return &state->base;
 }
 
@@ -344,12 +426,11 @@ struct drm_plane *omap_plane_init(struct drm_device *dev,
 		return ERR_PTR(-ENOMEM);
 
 	omap_plane->id = idx;
-	omap_plane->overlay = priv->overlays[idx];
 
 	DBG("%d: type=%d", omap_plane->id, type);
 	DBG("	crtc_mask: 0x%04x", possible_crtcs);
 
-	formats = dispc_ovl_get_color_modes(priv->dispc, omap_plane->overlay->id);
+	formats = dispc_ovl_get_color_modes(priv->dispc, omap_plane->id);
 	for (nformats = 0; formats[nformats]; ++nformats)
 		;
 
-- 
2.25.1

[PATCH v6 8/9] drm/omap: add plane_atomic_print_state support

[Neil Armstrong]

From: Benoit Parrot <bparrot@ti.com>

Now that we added specific item to our subclassed drm_plane_state
we can add omap_plane_atomic_print_state() helper to dump out our own
driver specific plane state.

Signed-off-by: Benoit Parrot <bparrot@ti.com>
Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
---
 drivers/gpu/drm/omapdrm/omap_plane.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/gpu/drm/omapdrm/omap_plane.c b/drivers/gpu/drm/omapdrm/omap_plane.c
index ce5ed45401fb..5001c8354e4f 100644
--- a/drivers/gpu/drm/omapdrm/omap_plane.c
+++ b/drivers/gpu/drm/omapdrm/omap_plane.c
@@ -348,6 +348,21 @@ omap_plane_atomic_duplicate_state(struct drm_plane *plane)
 	return &state->base;
 }
 
+static void omap_plane_atomic_print_state(struct drm_printer *p,
+					  const struct drm_plane_state *state)
+{
+	struct omap_plane_state *omap_state = to_omap_plane_state(state);
+
+	drm_printf(p, "\toverlay=%s\n", omap_state->overlay ?
+					omap_state->overlay->name : "(null)");
+	if (omap_state->overlay) {
+		drm_printf(p, "\t\tidx=%d\n", omap_state->overlay->idx);
+		drm_printf(p, "\t\toverlay_id=%d\n",
+			   omap_state->overlay->id);
+		drm_printf(p, "\t\tcaps=0x%x\n", omap_state->overlay->caps);
+	}
+}
+
 static int omap_plane_atomic_set_property(struct drm_plane *plane,
 					  struct drm_plane_state *state,
 					  struct drm_property *property,
@@ -387,6 +402,7 @@ static const struct drm_plane_funcs omap_plane_funcs = {
 	.atomic_destroy_state = drm_atomic_helper_plane_destroy_state,
 	.atomic_set_property = omap_plane_atomic_set_property,
 	.atomic_get_property = omap_plane_atomic_get_property,
+	.atomic_print_state = omap_plane_atomic_print_state,
 };
 
 static bool omap_plane_supports_yuv(struct drm_plane *plane)
-- 
2.25.1

[PATCH v6 9/9] drm/omap: Add a 'right overlay' to plane state

[Neil Armstrong]

From: Benoit Parrot <bparrot@ti.com>

If the drm_plane has a source width that's greater than the max width
supported by a single hw overlay, then we assign a 'r_overlay' to it in
omap_plane_atomic_check().

Both overlays should have the capabilities required to handle the source
framebuffer. The only parameters that vary between the left and right
hwoverlays are the src_w, crtc_w, src_x and crtc_x as we just even chop
the fb into left and right halves.

We also take care of not creating odd width size when dealing with YUV
formats.

Since both halves need to be 'appear' side by side the zpos is
recalculated when dealing with dual overlay cases so that the other
planes zpos is consistent.

Depending on user space usage it is possible that on occasion the number
of requested planes exceeds the numbers of overlays required to display
them. In that case a failure would be returned for the plane that cannot
be handled at that time. It is up to user space to make sure the H/W
resource are not over-subscribed.

Signed-off-by: Benoit Parrot <bparrot@ti.com>
Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
---
 drivers/gpu/drm/omapdrm/omap_drv.c     |  91 ++++++++++++++++++-
 drivers/gpu/drm/omapdrm/omap_fb.c      |  33 ++++++-
 drivers/gpu/drm/omapdrm/omap_fb.h      |   4 +-
 drivers/gpu/drm/omapdrm/omap_overlay.c |  23 ++++-
 drivers/gpu/drm/omapdrm/omap_overlay.h |   3 +-
 drivers/gpu/drm/omapdrm/omap_plane.c   | 120 +++++++++++++++++++++++--
 drivers/gpu/drm/omapdrm/omap_plane.h   |   1 +
 7 files changed, 263 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c
index c7912374d393..f088b6313950 100644
--- a/drivers/gpu/drm/omapdrm/omap_drv.c
+++ b/drivers/gpu/drm/omapdrm/omap_drv.c
@@ -117,6 +117,95 @@ static void omap_atomic_commit_tail(struct drm_atomic_state *old_state)
 	dispc_runtime_put(priv->dispc);
 }
 
+static int drm_atomic_state_normalized_zpos_cmp(const void *a, const void *b)
+{
+	const struct drm_plane_state *sa = *(struct drm_plane_state **)a;
+	const struct drm_plane_state *sb = *(struct drm_plane_state **)b;
+
+	if (sa->normalized_zpos != sb->normalized_zpos)
+		return sa->normalized_zpos - sb->normalized_zpos;
+	else
+		return sa->plane->base.id - sb->plane->base.id;
+}
+
+static int omap_atomic_update_normalize_zpos(struct drm_device *dev,
+					     struct drm_atomic_state *state)
+{
+	struct drm_crtc *crtc;
+	struct drm_crtc_state *old_state, *new_state;
+	struct drm_plane *plane;
+	int c, i, n, inc;
+	int total_planes = dev->mode_config.num_total_plane;
+	struct drm_plane_state **states;
+	int ret = 0;
+
+	states = kmalloc_array(total_planes, sizeof(*states), GFP_KERNEL);
+	if (!states)
+		return -ENOMEM;
+
+	for_each_oldnew_crtc_in_state(state, crtc, old_state, new_state, c) {
+		if (old_state->plane_mask == new_state->plane_mask &&
+		    !new_state->zpos_changed)
+			continue;
+
+		/* Reset plane increment and index value for every crtc */
+		n = 0;
+
+		/*
+		 * Normalization process might create new states for planes
+		 * which normalized_zpos has to be recalculated.
+		 */
+		drm_for_each_plane_mask(plane, dev, new_state->plane_mask) {
+			struct drm_plane_state *plane_state =
+				drm_atomic_get_plane_state(new_state->state,
+							   plane);
+			if (IS_ERR(plane_state)) {
+				ret = PTR_ERR(plane_state);
+				goto done;
+			}
+			states[n++] = plane_state;
+		}
+
+		sort(states, n, sizeof(*states),
+		     drm_atomic_state_normalized_zpos_cmp, NULL);
+
+		for (i = 0, inc = 0; i < n; i++) {
+			plane = states[i]->plane;
+
+			states[i]->normalized_zpos = i + inc;
+			DRM_DEBUG_ATOMIC("[PLANE:%d:%s] updated normalized zpos value %d\n",
+					 plane->base.id, plane->name,
+					 states[i]->normalized_zpos);
+
+			if (is_omap_plane_dual_overlay(states[i]))
+				inc++;
+		}
+		new_state->zpos_changed = true;
+	}
+
+done:
+	kfree(states);
+	return ret;
+}
+
+static int omap_atomic_check(struct drm_device *dev,
+			     struct drm_atomic_state *state)
+{
+	int ret;
+
+	ret = drm_atomic_helper_check(dev, state);
+	if (ret)
+		return ret;
+
+	if (dev->mode_config.normalize_zpos) {
+		ret = omap_atomic_update_normalize_zpos(dev, state);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
 static const struct drm_mode_config_helper_funcs omap_mode_config_helper_funcs = {
 	.atomic_commit_tail = omap_atomic_commit_tail,
 };
@@ -124,7 +213,7 @@ static const struct drm_mode_config_helper_funcs omap_mode_config_helper_funcs =
 static const struct drm_mode_config_funcs omap_mode_config_funcs = {
 	.fb_create = omap_framebuffer_create,
 	.output_poll_changed = drm_fb_helper_output_poll_changed,
-	.atomic_check = drm_atomic_helper_check,
+	.atomic_check = omap_atomic_check,
 	.atomic_commit = drm_atomic_helper_commit,
 };
 
diff --git a/drivers/gpu/drm/omapdrm/omap_fb.c b/drivers/gpu/drm/omapdrm/omap_fb.c
index 190afc564914..895e66b08a81 100644
--- a/drivers/gpu/drm/omapdrm/omap_fb.c
+++ b/drivers/gpu/drm/omapdrm/omap_fb.c
@@ -131,7 +131,9 @@ static u32 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 *info,
+		struct omap_overlay_info *r_info)
 {
 	struct omap_framebuffer *omap_fb = to_omap_framebuffer(fb);
 	const struct drm_format_info *format = omap_fb->format;
@@ -218,6 +220,35 @@ void omap_framebuffer_update_scanout(struct drm_framebuffer *fb,
 	} else {
 		info->p_uv_addr = 0;
 	}
+
+	if (r_info) {
+		info->width /= 2;
+		info->out_width /= 2;
+
+		*r_info = *info;
+
+		if (fb->format->is_yuv) {
+			if (info->width & 1) {
+				info->width++;
+				r_info->width--;
+			}
+
+			if (info->out_width & 1) {
+				info->out_width++;
+				r_info->out_width--;
+			}
+		}
+
+		r_info->pos_x = info->pos_x + info->out_width;
+
+		r_info->paddr =	get_linear_addr(fb, format, 0,
+						x + info->width, y);
+		if (fb->format->format == DRM_FORMAT_NV12) {
+			r_info->p_uv_addr =
+				get_linear_addr(fb, format, 1,
+						x + info->width, y);
+		}
+	}
 }
 
 /* pin, prepare for scanout: */
diff --git a/drivers/gpu/drm/omapdrm/omap_fb.h b/drivers/gpu/drm/omapdrm/omap_fb.h
index c0e19aed8220..b75f0b5ef1d8 100644
--- a/drivers/gpu/drm/omapdrm/omap_fb.h
+++ b/drivers/gpu/drm/omapdrm/omap_fb.h
@@ -26,7 +26,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 *info,
+		struct omap_overlay_info *r_info);
 bool omap_framebuffer_supports_rotation(struct drm_framebuffer *fb);
 void omap_framebuffer_describe(struct drm_framebuffer *fb, struct seq_file *m);
 
diff --git a/drivers/gpu/drm/omapdrm/omap_overlay.c b/drivers/gpu/drm/omapdrm/omap_overlay.c
index afb2b44fdf86..10730c9b2752 100644
--- a/drivers/gpu/drm/omapdrm/omap_overlay.c
+++ b/drivers/gpu/drm/omapdrm/omap_overlay.c
@@ -67,12 +67,13 @@ omap_plane_find_free_overlay(struct drm_device *dev, struct drm_plane *hwoverlay
  * next global overlay_map to be enabled when atomic transaction is valid.
  */
 int omap_overlay_assign(struct drm_atomic_state *s, struct drm_plane *plane,
-			u32 caps, u32 fourcc, struct omap_hw_overlay **overlay)
+			u32 caps, u32 fourcc, struct omap_hw_overlay **overlay,
+			struct omap_hw_overlay **r_overlay)
 {
 	/* Get the global state of the current atomic transaction */
 	struct omap_global_state *state = omap_get_global_state(s);
 	struct drm_plane **overlay_map = state->hwoverlay_to_plane;
-	struct omap_hw_overlay *ovl;
+	struct omap_hw_overlay *ovl, *r_ovl;
 
 	ovl = omap_plane_find_free_overlay(s->dev, overlay_map, caps, fourcc);
 	if (!ovl)
@@ -81,8 +82,26 @@ int omap_overlay_assign(struct drm_atomic_state *s, struct drm_plane *plane,
 	overlay_map[ovl->idx] = plane;
 	*overlay = ovl;
 
+	if (r_overlay) {
+		r_ovl = omap_plane_find_free_overlay(s->dev, overlay_map,
+						     caps, fourcc);
+		if (!r_ovl) {
+			overlay_map[r_ovl->idx] = NULL;
+			*overlay = NULL;
+			return -ENOMEM;
+		}
+
+		overlay_map[r_ovl->idx] = plane;
+		*r_overlay = r_ovl;
+	}
+
 	DBG("%s: assign to plane %s caps %x", ovl->name, plane->name, caps);
 
+	if (r_overlay) {
+		DBG("%s: assign to right of plane %s caps %x",
+		    r_ovl->name, plane->name, caps);
+	}
+
 	return 0;
 }
 
diff --git a/drivers/gpu/drm/omapdrm/omap_overlay.h b/drivers/gpu/drm/omapdrm/omap_overlay.h
index 38ca55e1e2d7..e36a43f35563 100644
--- a/drivers/gpu/drm/omapdrm/omap_overlay.h
+++ b/drivers/gpu/drm/omapdrm/omap_overlay.h
@@ -28,7 +28,8 @@ struct omap_hw_overlay {
 int omap_hwoverlays_init(struct omap_drm_private *priv);
 void omap_hwoverlays_destroy(struct omap_drm_private *priv);
 int omap_overlay_assign(struct drm_atomic_state *s, struct drm_plane *plane,
-			u32 caps, u32 fourcc, struct omap_hw_overlay **overlay);
+			u32 caps, u32 fourcc, struct omap_hw_overlay **overlay,
+			struct omap_hw_overlay **r_overlay);
 void omap_overlay_release(struct drm_atomic_state *s, struct omap_hw_overlay *overlay);
 void omap_overlay_update_state(struct omap_drm_private *priv, struct omap_hw_overlay *overlay);
 #endif /* __OMAPDRM_OVERLAY_H__ */
diff --git a/drivers/gpu/drm/omapdrm/omap_plane.c b/drivers/gpu/drm/omapdrm/omap_plane.c
index 5001c8354e4f..15cdff988dbc 100644
--- a/drivers/gpu/drm/omapdrm/omap_plane.c
+++ b/drivers/gpu/drm/omapdrm/omap_plane.c
@@ -24,6 +24,7 @@ struct omap_plane_state {
 	struct drm_plane_state base;
 
 	struct omap_hw_overlay *overlay;
+	struct omap_hw_overlay *r_overlay;  /* right overlay */
 };
 
 #define to_omap_plane(x) container_of(x, struct omap_plane, base)
@@ -33,6 +34,13 @@ struct omap_plane {
 	enum omap_plane_id id;
 };
 
+bool is_omap_plane_dual_overlay(struct drm_plane_state *state)
+{
+	struct omap_plane_state *omap_state = to_omap_plane_state(state);
+
+	return !!omap_state->r_overlay;
+}
+
 static int omap_plane_prepare_fb(struct drm_plane *plane,
 				 struct drm_plane_state *new_state)
 {
@@ -61,16 +69,21 @@ static void omap_plane_atomic_update(struct drm_plane *plane,
 									   plane);
 	struct omap_plane_state *new_omap_state;
 	struct omap_plane_state *old_omap_state;
-	struct omap_overlay_info info;
-	enum omap_plane_id ovl_id;
+	struct omap_overlay_info info, r_info;
+	enum omap_plane_id ovl_id, r_ovl_id;
 	int ret;
+	bool dual_ovl;
 
 	new_omap_state = to_omap_plane_state(new_state);
 	old_omap_state = to_omap_plane_state(old_state);
 
+	dual_ovl = is_omap_plane_dual_overlay(new_state);
+
 	/* Cleanup previously held overlay if needed */
 	if (old_omap_state->overlay)
 		omap_overlay_update_state(priv, old_omap_state->overlay);
+	if (old_omap_state->r_overlay)
+		omap_overlay_update_state(priv, old_omap_state->r_overlay);
 
 	if (!new_omap_state->overlay) {
 		DBG("[PLANE:%d:%s] no overlay attached", plane->base.id, plane->name);
@@ -93,8 +106,11 @@ static void omap_plane_atomic_update(struct drm_plane *plane,
 	info.color_encoding = new_state->color_encoding;
 	info.color_range = new_state->color_range;
 
+	r_info = info;
+
 	/* update scanout: */
-	omap_framebuffer_update_scanout(new_state->fb, new_state, &info);
+	omap_framebuffer_update_scanout(new_state->fb, new_state, &info,
+					dual_ovl ? &r_info : NULL);
 
 	DBG("%s: %dx%d -> %dx%d (%d)",
 			new_omap_state->overlay->name, info.width, info.height,
@@ -102,6 +118,23 @@ static void omap_plane_atomic_update(struct drm_plane *plane,
 	DBG("%d,%d %pad %pad", info.pos_x, info.pos_y,
 			&info.paddr, &info.p_uv_addr);
 
+	if (dual_ovl) {
+		r_ovl_id = new_omap_state->r_overlay->id;
+		/*
+		 * If the current plane uses 2 hw planes the very next
+		 * zorder is used by the r_overlay so we just use the
+		 * main overlay zorder + 1
+		 */
+		r_info.zorder = info.zorder + 1;
+
+		DBG("%s: %dx%d -> %dx%d (%d)",
+		    new_omap_state->r_overlay->name,
+		    r_info.width, r_info.height,
+		    r_info.out_width, r_info.out_height, r_info.screen_width);
+		DBG("%d,%d %pad %pad", r_info.pos_x, r_info.pos_y,
+		    &r_info.paddr, &r_info.p_uv_addr);
+	}
+
 	/* and finally, update omapdss: */
 	ret = dispc_ovl_setup(priv->dispc, ovl_id, &info,
 			      omap_crtc_timings(new_state->crtc), false,
@@ -114,6 +147,21 @@ static void omap_plane_atomic_update(struct drm_plane *plane,
 	}
 
 	dispc_ovl_enable(priv->dispc, ovl_id, true);
+
+	if (dual_ovl) {
+		ret = dispc_ovl_setup(priv->dispc, r_ovl_id, &r_info,
+				      omap_crtc_timings(new_state->crtc), false,
+				      omap_crtc_channel(new_state->crtc));
+		if (ret) {
+			dev_err(plane->dev->dev, "Failed to setup plane right-overlay %s\n",
+				plane->name);
+			dispc_ovl_enable(priv->dispc, r_ovl_id, false);
+			dispc_ovl_enable(priv->dispc, ovl_id, false);
+			return;
+		}
+
+		dispc_ovl_enable(priv->dispc, r_ovl_id, true);
+	}
 }
 
 static void omap_plane_atomic_disable(struct drm_plane *plane,
@@ -139,6 +187,11 @@ static void omap_plane_atomic_disable(struct drm_plane *plane,
 
 	omap_overlay_update_state(priv, old_omap_state->overlay);
 	new_omap_state->overlay = NULL;
+
+	if (is_omap_plane_dual_overlay(old_state)) {
+		omap_overlay_update_state(priv, old_omap_state->r_overlay);
+		new_omap_state->r_overlay = NULL;
+	}
 }
 
 #define FRAC_16_16(mult, div)    (((mult) << 16) / (div))
@@ -154,6 +207,7 @@ static int omap_plane_atomic_check(struct drm_plane *plane,
 	struct omap_plane_state *omap_state = to_omap_plane_state(new_plane_state);
 	struct omap_global_state *omap_overlay_global_state;
 	struct drm_crtc_state *crtc_state;
+	bool new_r_hw_overlay = false;
 	bool new_hw_overlay = false;
 	u32 max_width, max_height;
 	struct drm_crtc *crtc;
@@ -196,7 +250,9 @@ static int omap_plane_atomic_check(struct drm_plane *plane,
 
 	if (!new_plane_state->visible) {
 		omap_overlay_release(state, omap_state->overlay);
+		omap_overlay_release(state, omap_state->r_overlay);
 		omap_state->overlay = NULL;
+		omap_state->r_overlay = NULL;
 		return 0;
 	}
 
@@ -213,8 +269,30 @@ static int omap_plane_atomic_check(struct drm_plane *plane,
 	if (new_plane_state->src_h > max_height || new_plane_state->crtc_h > height)
 		return -EINVAL;
 
-	if (new_plane_state->src_w > max_width || new_plane_state->crtc_w > width)
-		return -EINVAL;
+
+	if (new_plane_state->src_w > max_width || new_plane_state->crtc_w > width) {
+		bool is_fourcc_yuv = new_plane_state->fb->format->is_yuv;
+
+		if (is_fourcc_yuv && (((new_plane_state->src_w >> 16) / 2 & 1) ||
+				      new_plane_state->crtc_w / 2 & 1)) {
+			/*
+			 * When calculating the split overlay width
+			 * and it yield an odd value we will need to adjust
+			 * the indivual width +/- 1. So make sure it fits
+			 */
+			if (new_plane_state->src_w <= ((2 * width - 1) << 16) &&
+			    new_plane_state->crtc_w <= (2 * width - 1))
+				new_r_hw_overlay = true;
+			else
+				return -EINVAL;
+		} else {
+			if (new_plane_state->src_w <= (2 * max_width) &&
+			    new_plane_state->crtc_w <= (2 * width))
+				new_r_hw_overlay = true;
+			else
+				return -EINVAL;
+		}
+	}
 
 	if (new_plane_state->rotation != DRM_MODE_ROTATE_0 &&
 	    !omap_framebuffer_supports_rotation(new_plane_state->fb))
@@ -239,24 +317,44 @@ static int omap_plane_atomic_check(struct drm_plane *plane,
 			new_hw_overlay = true;
 	}
 
+	/*
+	 * check if we need two overlays and only have 1 or
+	 * if we had 2 overlays but will only need 1
+	 */
+	if ((new_r_hw_overlay && !omap_state->r_overlay) ||
+	    (!new_r_hw_overlay && omap_state->r_overlay))
+		new_hw_overlay = true;
+
 	if (new_hw_overlay) {
 		struct omap_hw_overlay *old_ovl = omap_state->overlay;
+		struct omap_hw_overlay *old_r_ovl = omap_state->r_overlay;
 		struct omap_hw_overlay *new_ovl = NULL;
+		struct omap_hw_overlay *new_r_ovl = NULL;
 
 		omap_overlay_release(state, old_ovl);
+		omap_overlay_release(state, old_r_ovl);
 
-		ret = omap_overlay_assign(state, plane, caps, fourcc, &new_ovl);
+		ret = omap_overlay_assign(state, plane, caps, fourcc, &new_ovl,
+					  new_r_hw_overlay ? &new_r_ovl : NULL);
 		if (ret) {
 			DBG("%s: failed to assign hw_overlay", plane->name);
 			omap_state->overlay = NULL;
+			omap_state->r_overlay = NULL;
 			return ret;
 		}
 
 		omap_state->overlay = new_ovl;
+		if (new_r_hw_overlay)
+			omap_state->r_overlay = new_r_ovl;
+		else
+			omap_state->r_overlay = NULL;
 	}
 
 	DBG("plane: %s overlay_id: %d", plane->name, omap_state->overlay->id);
 
+	if (omap_state->r_overlay)
+		DBG("plane: %s r_overlay_id: %d", plane->name, omap_state->r_overlay->id);
+
 	return 0;
 }
 
@@ -344,6 +442,7 @@ omap_plane_atomic_duplicate_state(struct drm_plane *plane)
 	__drm_atomic_helper_plane_duplicate_state(plane, &state->base);
 
 	state->overlay = current_state->overlay;
+	state->r_overlay = current_state->r_overlay;
 
 	return &state->base;
 }
@@ -361,6 +460,15 @@ static void omap_plane_atomic_print_state(struct drm_printer *p,
 			   omap_state->overlay->id);
 		drm_printf(p, "\t\tcaps=0x%x\n", omap_state->overlay->caps);
 	}
+
+	drm_printf(p, "\tr_overlay=%s\n", omap_state->r_overlay ?
+					  omap_state->r_overlay->name : "(null)");
+	if (omap_state->r_overlay) {
+		drm_printf(p, "\t\tidx=%d\n", omap_state->r_overlay->idx);
+		drm_printf(p, "\t\toverlay_id=%d\n",
+			   omap_state->r_overlay->id);
+		drm_printf(p, "\t\tcaps=0x%x\n", omap_state->r_overlay->caps);
+	}
 }
 
 static int omap_plane_atomic_set_property(struct drm_plane *plane,
diff --git a/drivers/gpu/drm/omapdrm/omap_plane.h b/drivers/gpu/drm/omapdrm/omap_plane.h
index 0c28fe8ffa20..a9a33e12722a 100644
--- a/drivers/gpu/drm/omapdrm/omap_plane.h
+++ b/drivers/gpu/drm/omapdrm/omap_plane.h
@@ -22,5 +22,6 @@ struct drm_plane *omap_plane_init(struct drm_device *dev,
 		u32 possible_crtcs);
 void omap_plane_install_properties(struct drm_plane *plane,
 		struct drm_mode_object *obj);
+bool is_omap_plane_dual_overlay(struct drm_plane_state *state);
 
 #endif /* __OMAPDRM_PLANE_H__ */
-- 
2.25.1

Re: [PATCH v6 1/9] drm/omap: add sanity plane state check

[Tomi Valkeinen]

On 18/10/2021 17:28, Neil Armstrong wrote:
> Call drm_atomic_helper_check_plane_state() from the plane
> atomic_check() callback in order to add plane state sanity
> checking.
> 
> It will permit filtering out totally bad scaling factors, even
> if the real check are done later in the atomic commit.

I think there's more to it: the function sets plane_state->visible, 
which is used in later patches.

  Tomi

Re: [PATCH v6 1/9] drm/omap: add sanity plane state check

[Tomi Valkeinen]

On 27/10/2021 11:29, Tomi Valkeinen wrote:
> On 18/10/2021 17:28, Neil Armstrong wrote:
>> Call drm_atomic_helper_check_plane_state() from the plane
>> atomic_check() callback in order to add plane state sanity
>> checking.
>>
>> It will permit filtering out totally bad scaling factors, even
>> if the real check are done later in the atomic commit.
> 
> I think there's more to it: the function sets plane_state->visible, 
> which is used in later patches.

Maybe that could be mentioned in a comment. But otherwise:

Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>

  Tomi

Re: [PATCH v6 2/9] drm/omap: Add ability to check if requested plane modes can be supported

[Tomi Valkeinen]

On 18/10/2021 17:28, Neil Armstrong wrote:
> From: Benoit Parrot <bparrot@ti.com>
> 
> We currently assume that an overlay has the same maximum width and
> maximum height as the overlay manager. This assumption is incorrect. On
> some variants the overlay manager maximum width is twice the maximum
> width that the overlay can handle. We need to add the appropriate data
> per variant as well as export a helper function to retrieve the data so
> check can be made dynamically in omap_plane_atomic_check().
> 
> Signed-off-by: Benoit Parrot <bparrot@ti.com>
> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
> ---
>   drivers/gpu/drm/omapdrm/dss/dispc.c  | 22 ++++++++++++++++++++++
>   drivers/gpu/drm/omapdrm/dss/dss.h    |  2 ++
>   drivers/gpu/drm/omapdrm/omap_plane.c | 14 ++++++++++++++
>   3 files changed, 38 insertions(+)

Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>

  Tomi

Re: [PATCH v6 3/9] drm/omap: Add ovl checking funcs to dispc_ops

[Tomi Valkeinen]

On 18/10/2021 17:28, Neil Armstrong wrote:
> From: Benoit Parrot <bparrot@ti.com>
> 
> In order to be able to dynamically assign overlays to planes we need to
> be able to asses the overlay capabilities.
> 
> Add a helper function to be able to retrieve the supported capabilities
> of an overlay.
> 
> And export the function to check if a fourcc is supported on a given
> overlay.
> 
> Signed-off-by: Benoit Parrot <bparrot@ti.com>
> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
> ---
>   drivers/gpu/drm/omapdrm/dss/dispc.c | 9 +++++++--
>   drivers/gpu/drm/omapdrm/dss/dss.h   | 3 +++
>   2 files changed, 10 insertions(+), 2 deletions(-)

Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>

  Tomi

Re: [PATCH v6 4/9] drm/omap: introduce omap_hw_overlay

[Tomi Valkeinen]

On 18/10/2021 17:28, Neil Armstrong wrote:
> From: Benoit Parrot <bparrot@ti.com>
> 
> Split out the hardware overlay specifics from omap_plane.
> To start, the hw overlays are statically assigned to planes.
> 
> The goal is to eventually assign hw overlays dynamically to planes
> during plane->atomic_check() based on requested caps (scaling, YUV,
> etc). And then perform hw overlay re-assignment if required.
> 
> Signed-off-by: Benoit Parrot <bparrot@ti.com>
> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
> ---
>   drivers/gpu/drm/omapdrm/Makefile       |  1 +
>   drivers/gpu/drm/omapdrm/omap_drv.c     |  9 ++-
>   drivers/gpu/drm/omapdrm/omap_drv.h     |  4 ++
>   drivers/gpu/drm/omapdrm/omap_overlay.c | 84 ++++++++++++++++++++++++++
>   drivers/gpu/drm/omapdrm/omap_overlay.h | 30 +++++++++
>   drivers/gpu/drm/omapdrm/omap_plane.c   | 54 +++++++----------
>   6 files changed, 148 insertions(+), 34 deletions(-)
>   create mode 100644 drivers/gpu/drm/omapdrm/omap_overlay.c
>   create mode 100644 drivers/gpu/drm/omapdrm/omap_overlay.h

Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>

  Tomi

Re: [PATCH v6 5/9] drm/omap: omap_plane: subclass drm_plane_state

[Tomi Valkeinen]

On 18/10/2021 17:28, Neil Armstrong wrote:
> From: Benoit Parrot <bparrot@ti.com>
> 
> In preparation to add omap plane state specific extensions we need to
> subclass drm_plane_state and add the relevant helpers.
> 
> The addition of specific extension will be done separately.
> 
> Signed-off-by: Benoit Parrot <bparrot@ti.com>
> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
> ---
>   drivers/gpu/drm/omapdrm/omap_plane.c | 36 +++++++++++++++++++++++++---
>   1 file changed, 33 insertions(+), 3 deletions(-)

Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>

  Tomi

Re: [PATCH v6 8/9] drm/omap: add plane_atomic_print_state support

[Tomi Valkeinen]

On 18/10/2021 17:28, Neil Armstrong wrote:
> From: Benoit Parrot <bparrot@ti.com>
> 
> Now that we added specific item to our subclassed drm_plane_state
> we can add omap_plane_atomic_print_state() helper to dump out our own
> driver specific plane state.
> 
> Signed-off-by: Benoit Parrot <bparrot@ti.com>
> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
> ---
>   drivers/gpu/drm/omapdrm/omap_plane.c | 16 ++++++++++++++++
>   1 file changed, 16 insertions(+)
> 
> diff --git a/drivers/gpu/drm/omapdrm/omap_plane.c b/drivers/gpu/drm/omapdrm/omap_plane.c
> index ce5ed45401fb..5001c8354e4f 100644
> --- a/drivers/gpu/drm/omapdrm/omap_plane.c
> +++ b/drivers/gpu/drm/omapdrm/omap_plane.c
> @@ -348,6 +348,21 @@ omap_plane_atomic_duplicate_state(struct drm_plane *plane)
>   	return &state->base;
>   }
>   
> +static void omap_plane_atomic_print_state(struct drm_printer *p,
> +					  const struct drm_plane_state *state)
> +{
> +	struct omap_plane_state *omap_state = to_omap_plane_state(state);
> +
> +	drm_printf(p, "\toverlay=%s\n", omap_state->overlay ?
> +					omap_state->overlay->name : "(null)");
> +	if (omap_state->overlay) {
> +		drm_printf(p, "\t\tidx=%d\n", omap_state->overlay->idx);
> +		drm_printf(p, "\t\toverlay_id=%d\n",
> +			   omap_state->overlay->id);
> +		drm_printf(p, "\t\tcaps=0x%x\n", omap_state->overlay->caps);
> +	}
> +}

This prints:

         overlay=gfx
                 idx=0
                 overlay_id=0
                 caps=0x3e

I'm not sure if some of these details are needed. The name ("gfx") and 
overlay_id refer to the same thing, and while idx is in theory a 
different value, in practice it's always the same as overlay_id. And 
even if it was a different number, I think idx is kind of irrelevant, 
isn't it?

caps can be figured out from the name of the overlay, but perhaps it 
doesn't hurt to print them here. Then again, if none of the other debug 
prints show the cap values (e.g. "requires cap 0x4), maybe printing the 
caps value is not really useful here.

Maybe this could be just a single line, say:

overlay=gfx

or if caps is useful:

overlay=gfx (caps=0x3e)

What do you think?

  Tomi

Re: [PATCH v6 8/9] drm/omap: add plane_atomic_print_state support

[Neil Armstrong]

On 27/10/2021 14:23, Tomi Valkeinen wrote:
> On 18/10/2021 17:28, Neil Armstrong wrote:
>> From: Benoit Parrot <bparrot@ti.com>
>>
>> Now that we added specific item to our subclassed drm_plane_state
>> we can add omap_plane_atomic_print_state() helper to dump out our own
>> driver specific plane state.
>>
>> Signed-off-by: Benoit Parrot <bparrot@ti.com>
>> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
>> ---
>>   drivers/gpu/drm/omapdrm/omap_plane.c | 16 ++++++++++++++++
>>   1 file changed, 16 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/omapdrm/omap_plane.c b/drivers/gpu/drm/omapdrm/omap_plane.c
>> index ce5ed45401fb..5001c8354e4f 100644
>> --- a/drivers/gpu/drm/omapdrm/omap_plane.c
>> +++ b/drivers/gpu/drm/omapdrm/omap_plane.c
>> @@ -348,6 +348,21 @@ omap_plane_atomic_duplicate_state(struct drm_plane *plane)
>>       return &state->base;
>>   }
>>   +static void omap_plane_atomic_print_state(struct drm_printer *p,
>> +                      const struct drm_plane_state *state)
>> +{
>> +    struct omap_plane_state *omap_state = to_omap_plane_state(state);
>> +
>> +    drm_printf(p, "\toverlay=%s\n", omap_state->overlay ?
>> +                    omap_state->overlay->name : "(null)");
>> +    if (omap_state->overlay) {
>> +        drm_printf(p, "\t\tidx=%d\n", omap_state->overlay->idx);
>> +        drm_printf(p, "\t\toverlay_id=%d\n",
>> +               omap_state->overlay->id);
>> +        drm_printf(p, "\t\tcaps=0x%x\n", omap_state->overlay->caps);
>> +    }
>> +}
> 
> This prints:
> 
>         overlay=gfx
>                 idx=0
>                 overlay_id=0
>                 caps=0x3e
> 
> I'm not sure if some of these details are needed. The name ("gfx") and overlay_id refer to the same thing, and while idx is in theory a different value, in practice it's always the same as overlay_id. And even if it was a different number, I think idx is kind of irrelevant, isn't it?
> 
> caps can be figured out from the name of the overlay, but perhaps it doesn't hurt to print them here. Then again, if none of the other debug prints show the cap values (e.g. "requires cap 0x4), maybe printing the caps value is not really useful here.
> 
> Maybe this could be just a single line, say:
> 
> overlay=gfx
> 
> or if caps is useful:
> 
> overlay=gfx (caps=0x3e)
> 
> What do you think?

I'm ok with that.

Thanks,
Neil

> 
>  Tomi

Re: [PATCH v6 9/9] drm/omap: Add a 'right overlay' to plane state

[Tomi Valkeinen]

On 18/10/2021 17:28, Neil Armstrong wrote:
> From: Benoit Parrot <bparrot@ti.com>
> 
> If the drm_plane has a source width that's greater than the max width
> supported by a single hw overlay, then we assign a 'r_overlay' to it in
> omap_plane_atomic_check().
> 
> Both overlays should have the capabilities required to handle the source
> framebuffer. The only parameters that vary between the left and right
> hwoverlays are the src_w, crtc_w, src_x and crtc_x as we just even chop
> the fb into left and right halves.
> 
> We also take care of not creating odd width size when dealing with YUV
> formats.
> 
> Since both halves need to be 'appear' side by side the zpos is
> recalculated when dealing with dual overlay cases so that the other
> planes zpos is consistent.
> 
> Depending on user space usage it is possible that on occasion the number
> of requested planes exceeds the numbers of overlays required to display
> them. In that case a failure would be returned for the plane that cannot
> be handled at that time. It is up to user space to make sure the H/W
> resource are not over-subscribed.
> 
> Signed-off-by: Benoit Parrot <bparrot@ti.com>
> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
> ---
>   drivers/gpu/drm/omapdrm/omap_drv.c     |  91 ++++++++++++++++++-
>   drivers/gpu/drm/omapdrm/omap_fb.c      |  33 ++++++-
>   drivers/gpu/drm/omapdrm/omap_fb.h      |   4 +-
>   drivers/gpu/drm/omapdrm/omap_overlay.c |  23 ++++-
>   drivers/gpu/drm/omapdrm/omap_overlay.h |   3 +-
>   drivers/gpu/drm/omapdrm/omap_plane.c   | 120 +++++++++++++++++++++++--
>   drivers/gpu/drm/omapdrm/omap_plane.h   |   1 +
>   7 files changed, 263 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c
> index c7912374d393..f088b6313950 100644
> --- a/drivers/gpu/drm/omapdrm/omap_drv.c
> +++ b/drivers/gpu/drm/omapdrm/omap_drv.c
> @@ -117,6 +117,95 @@ static void omap_atomic_commit_tail(struct drm_atomic_state *old_state)
>   	dispc_runtime_put(priv->dispc);
>   }
>   
> +static int drm_atomic_state_normalized_zpos_cmp(const void *a, const void *b)
> +{
> +	const struct drm_plane_state *sa = *(struct drm_plane_state **)a;
> +	const struct drm_plane_state *sb = *(struct drm_plane_state **)b;
> +
> +	if (sa->normalized_zpos != sb->normalized_zpos)
> +		return sa->normalized_zpos - sb->normalized_zpos;
> +	else
> +		return sa->plane->base.id - sb->plane->base.id;
> +}

I think this, or the function below, needs a comment. I don't think it's 
obvious what's the logic here. It's mostly a copy of 
drm_atomic_normalize_zpos and drm_atomic_helper_crtc_normalize_zpos, if 
I'm not mistaken, it migth be good to point out what the difference is.

I'm also wondering if these normalize_zpos functions should be split to 
a separate patch (without the is_omap_plane_dual_overlay call part).

> +static int omap_atomic_update_normalize_zpos(struct drm_device *dev,
> +					     struct drm_atomic_state *state)
> +{
> +	struct drm_crtc *crtc;
> +	struct drm_crtc_state *old_state, *new_state;
> +	struct drm_plane *plane;
> +	int c, i, n, inc;
> +	int total_planes = dev->mode_config.num_total_plane;
> +	struct drm_plane_state **states;
> +	int ret = 0;
> +
> +	states = kmalloc_array(total_planes, sizeof(*states), GFP_KERNEL);
> +	if (!states)
> +		return -ENOMEM;
> +
> +	for_each_oldnew_crtc_in_state(state, crtc, old_state, new_state, c) {
> +		if (old_state->plane_mask == new_state->plane_mask &&
> +		    !new_state->zpos_changed)
> +			continue;
> +
> +		/* Reset plane increment and index value for every crtc */
> +		n = 0;
> +
> +		/*
> +		 * Normalization process might create new states for planes
> +		 * which normalized_zpos has to be recalculated.
> +		 */
> +		drm_for_each_plane_mask(plane, dev, new_state->plane_mask) {
> +			struct drm_plane_state *plane_state =
> +				drm_atomic_get_plane_state(new_state->state,
> +							   plane);
> +			if (IS_ERR(plane_state)) {
> +				ret = PTR_ERR(plane_state);
> +				goto done;
> +			}
> +			states[n++] = plane_state;
> +		}
> +
> +		sort(states, n, sizeof(*states),
> +		     drm_atomic_state_normalized_zpos_cmp, NULL);
> +
> +		for (i = 0, inc = 0; i < n; i++) {
> +			plane = states[i]->plane;
> +
> +			states[i]->normalized_zpos = i + inc;
> +			DRM_DEBUG_ATOMIC("[PLANE:%d:%s] updated normalized zpos value %d\n",
> +					 plane->base.id, plane->name,
> +					 states[i]->normalized_zpos);
> +
> +			if (is_omap_plane_dual_overlay(states[i]))
> +				inc++;
> +		}
> +		new_state->zpos_changed = true;
> +	}
> +
> +done:
> +	kfree(states);
> +	return ret;
> +}
> +
> +static int omap_atomic_check(struct drm_device *dev,
> +			     struct drm_atomic_state *state)
> +{
> +	int ret;
> +
> +	ret = drm_atomic_helper_check(dev, state);
> +	if (ret)
> +		return ret;
> +
> +	if (dev->mode_config.normalize_zpos) {
> +		ret = omap_atomic_update_normalize_zpos(dev, state);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
>   static const struct drm_mode_config_helper_funcs omap_mode_config_helper_funcs = {
>   	.atomic_commit_tail = omap_atomic_commit_tail,
>   };
> @@ -124,7 +213,7 @@ static const struct drm_mode_config_helper_funcs omap_mode_config_helper_funcs =
>   static const struct drm_mode_config_funcs omap_mode_config_funcs = {
>   	.fb_create = omap_framebuffer_create,
>   	.output_poll_changed = drm_fb_helper_output_poll_changed,
> -	.atomic_check = drm_atomic_helper_check,
> +	.atomic_check = omap_atomic_check,
>   	.atomic_commit = drm_atomic_helper_commit,
>   };
>   
> diff --git a/drivers/gpu/drm/omapdrm/omap_fb.c b/drivers/gpu/drm/omapdrm/omap_fb.c
> index 190afc564914..895e66b08a81 100644
> --- a/drivers/gpu/drm/omapdrm/omap_fb.c
> +++ b/drivers/gpu/drm/omapdrm/omap_fb.c
> @@ -131,7 +131,9 @@ static u32 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 *info,
> +		struct omap_overlay_info *r_info)
>   {
>   	struct omap_framebuffer *omap_fb = to_omap_framebuffer(fb);
>   	const struct drm_format_info *format = omap_fb->format;
> @@ -218,6 +220,35 @@ void omap_framebuffer_update_scanout(struct drm_framebuffer *fb,
>   	} else {
>   		info->p_uv_addr = 0;
>   	}
> +
> +	if (r_info) {
> +		info->width /= 2;
> +		info->out_width /= 2;
> +
> +		*r_info = *info;
> +
> +		if (fb->format->is_yuv) {
> +			if (info->width & 1) {
> +				info->width++;
> +				r_info->width--;
> +			}
> +
> +			if (info->out_width & 1) {
> +				info->out_width++;
> +				r_info->out_width--;
> +			}
> +		}
> +
> +		r_info->pos_x = info->pos_x + info->out_width;
> +
> +		r_info->paddr =	get_linear_addr(fb, format, 0,
> +						x + info->width, y);
> +		if (fb->format->format == DRM_FORMAT_NV12) {
> +			r_info->p_uv_addr =
> +				get_linear_addr(fb, format, 1,
> +						x + info->width, y);
> +		}
> +	}
>   }
>   
>   /* pin, prepare for scanout: */
> diff --git a/drivers/gpu/drm/omapdrm/omap_fb.h b/drivers/gpu/drm/omapdrm/omap_fb.h
> index c0e19aed8220..b75f0b5ef1d8 100644
> --- a/drivers/gpu/drm/omapdrm/omap_fb.h
> +++ b/drivers/gpu/drm/omapdrm/omap_fb.h
> @@ -26,7 +26,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 *info,
> +		struct omap_overlay_info *r_info);
>   bool omap_framebuffer_supports_rotation(struct drm_framebuffer *fb);
>   void omap_framebuffer_describe(struct drm_framebuffer *fb, struct seq_file *m);
>   
> diff --git a/drivers/gpu/drm/omapdrm/omap_overlay.c b/drivers/gpu/drm/omapdrm/omap_overlay.c
> index afb2b44fdf86..10730c9b2752 100644
> --- a/drivers/gpu/drm/omapdrm/omap_overlay.c
> +++ b/drivers/gpu/drm/omapdrm/omap_overlay.c
> @@ -67,12 +67,13 @@ omap_plane_find_free_overlay(struct drm_device *dev, struct drm_plane *hwoverlay
>    * next global overlay_map to be enabled when atomic transaction is valid.
>    */
>   int omap_overlay_assign(struct drm_atomic_state *s, struct drm_plane *plane,
> -			u32 caps, u32 fourcc, struct omap_hw_overlay **overlay)
> +			u32 caps, u32 fourcc, struct omap_hw_overlay **overlay,
> +			struct omap_hw_overlay **r_overlay)
>   {
>   	/* Get the global state of the current atomic transaction */
>   	struct omap_global_state *state = omap_get_global_state(s);
>   	struct drm_plane **overlay_map = state->hwoverlay_to_plane;
> -	struct omap_hw_overlay *ovl;
> +	struct omap_hw_overlay *ovl, *r_ovl;
>   
>   	ovl = omap_plane_find_free_overlay(s->dev, overlay_map, caps, fourcc);
>   	if (!ovl)
> @@ -81,8 +82,26 @@ int omap_overlay_assign(struct drm_atomic_state *s, struct drm_plane *plane,
>   	overlay_map[ovl->idx] = plane;
>   	*overlay = ovl;
>   
> +	if (r_overlay) {
> +		r_ovl = omap_plane_find_free_overlay(s->dev, overlay_map,
> +						     caps, fourcc);
> +		if (!r_ovl) {
> +			overlay_map[r_ovl->idx] = NULL;
> +			*overlay = NULL;
> +			return -ENOMEM;
> +		}
> +
> +		overlay_map[r_ovl->idx] = plane;
> +		*r_overlay = r_ovl;
> +	}
> +
>   	DBG("%s: assign to plane %s caps %x", ovl->name, plane->name, caps);
>   
> +	if (r_overlay) {
> +		DBG("%s: assign to right of plane %s caps %x",
> +		    r_ovl->name, plane->name, caps);
> +	}
> +
>   	return 0;
>   }
>   
> diff --git a/drivers/gpu/drm/omapdrm/omap_overlay.h b/drivers/gpu/drm/omapdrm/omap_overlay.h
> index 38ca55e1e2d7..e36a43f35563 100644
> --- a/drivers/gpu/drm/omapdrm/omap_overlay.h
> +++ b/drivers/gpu/drm/omapdrm/omap_overlay.h
> @@ -28,7 +28,8 @@ struct omap_hw_overlay {
>   int omap_hwoverlays_init(struct omap_drm_private *priv);
>   void omap_hwoverlays_destroy(struct omap_drm_private *priv);
>   int omap_overlay_assign(struct drm_atomic_state *s, struct drm_plane *plane,
> -			u32 caps, u32 fourcc, struct omap_hw_overlay **overlay);
> +			u32 caps, u32 fourcc, struct omap_hw_overlay **overlay,
> +			struct omap_hw_overlay **r_overlay);
>   void omap_overlay_release(struct drm_atomic_state *s, struct omap_hw_overlay *overlay);
>   void omap_overlay_update_state(struct omap_drm_private *priv, struct omap_hw_overlay *overlay);
>   #endif /* __OMAPDRM_OVERLAY_H__ */
> diff --git a/drivers/gpu/drm/omapdrm/omap_plane.c b/drivers/gpu/drm/omapdrm/omap_plane.c
> index 5001c8354e4f..15cdff988dbc 100644
> --- a/drivers/gpu/drm/omapdrm/omap_plane.c
> +++ b/drivers/gpu/drm/omapdrm/omap_plane.c
> @@ -24,6 +24,7 @@ struct omap_plane_state {
>   	struct drm_plane_state base;
>   
>   	struct omap_hw_overlay *overlay;
> +	struct omap_hw_overlay *r_overlay;  /* right overlay */
>   };
>   
>   #define to_omap_plane(x) container_of(x, struct omap_plane, base)
> @@ -33,6 +34,13 @@ struct omap_plane {
>   	enum omap_plane_id id;
>   };
>   
> +bool is_omap_plane_dual_overlay(struct drm_plane_state *state)
> +{
> +	struct omap_plane_state *omap_state = to_omap_plane_state(state);
> +
> +	return !!omap_state->r_overlay;
> +}
> +
>   static int omap_plane_prepare_fb(struct drm_plane *plane,
>   				 struct drm_plane_state *new_state)
>   {
> @@ -61,16 +69,21 @@ static void omap_plane_atomic_update(struct drm_plane *plane,
>   									   plane);
>   	struct omap_plane_state *new_omap_state;
>   	struct omap_plane_state *old_omap_state;
> -	struct omap_overlay_info info;
> -	enum omap_plane_id ovl_id;
> +	struct omap_overlay_info info, r_info;
> +	enum omap_plane_id ovl_id, r_ovl_id;
>   	int ret;
> +	bool dual_ovl;
>   
>   	new_omap_state = to_omap_plane_state(new_state);
>   	old_omap_state = to_omap_plane_state(old_state);
>   
> +	dual_ovl = is_omap_plane_dual_overlay(new_state);
> +
>   	/* Cleanup previously held overlay if needed */
>   	if (old_omap_state->overlay)
>   		omap_overlay_update_state(priv, old_omap_state->overlay);
> +	if (old_omap_state->r_overlay)
> +		omap_overlay_update_state(priv, old_omap_state->r_overlay);
>   
>   	if (!new_omap_state->overlay) {
>   		DBG("[PLANE:%d:%s] no overlay attached", plane->base.id, plane->name);
> @@ -93,8 +106,11 @@ static void omap_plane_atomic_update(struct drm_plane *plane,
>   	info.color_encoding = new_state->color_encoding;
>   	info.color_range = new_state->color_range;
>   
> +	r_info = info;
> +
>   	/* update scanout: */
> -	omap_framebuffer_update_scanout(new_state->fb, new_state, &info);
> +	omap_framebuffer_update_scanout(new_state->fb, new_state, &info,
> +					dual_ovl ? &r_info : NULL);
>   
>   	DBG("%s: %dx%d -> %dx%d (%d)",
>   			new_omap_state->overlay->name, info.width, info.height,
> @@ -102,6 +118,23 @@ static void omap_plane_atomic_update(struct drm_plane *plane,
>   	DBG("%d,%d %pad %pad", info.pos_x, info.pos_y,
>   			&info.paddr, &info.p_uv_addr);
>   
> +	if (dual_ovl) {
> +		r_ovl_id = new_omap_state->r_overlay->id;
> +		/*
> +		 * If the current plane uses 2 hw planes the very next
> +		 * zorder is used by the r_overlay so we just use the
> +		 * main overlay zorder + 1
> +		 */
> +		r_info.zorder = info.zorder + 1;
> +
> +		DBG("%s: %dx%d -> %dx%d (%d)",
> +		    new_omap_state->r_overlay->name,
> +		    r_info.width, r_info.height,
> +		    r_info.out_width, r_info.out_height, r_info.screen_width);
> +		DBG("%d,%d %pad %pad", r_info.pos_x, r_info.pos_y,
> +		    &r_info.paddr, &r_info.p_uv_addr);
> +	}
> +
>   	/* and finally, update omapdss: */
>   	ret = dispc_ovl_setup(priv->dispc, ovl_id, &info,
>   			      omap_crtc_timings(new_state->crtc), false,
> @@ -114,6 +147,21 @@ static void omap_plane_atomic_update(struct drm_plane *plane,
>   	}
>   
>   	dispc_ovl_enable(priv->dispc, ovl_id, true);
> +
> +	if (dual_ovl) {
> +		ret = dispc_ovl_setup(priv->dispc, r_ovl_id, &r_info,
> +				      omap_crtc_timings(new_state->crtc), false,
> +				      omap_crtc_channel(new_state->crtc));
> +		if (ret) {
> +			dev_err(plane->dev->dev, "Failed to setup plane right-overlay %s\n",
> +				plane->name);
> +			dispc_ovl_enable(priv->dispc, r_ovl_id, false);
> +			dispc_ovl_enable(priv->dispc, ovl_id, false);
> +			return;
> +		}
> +
> +		dispc_ovl_enable(priv->dispc, r_ovl_id, true);
> +	}
>   }
>   
>   static void omap_plane_atomic_disable(struct drm_plane *plane,
> @@ -139,6 +187,11 @@ static void omap_plane_atomic_disable(struct drm_plane *plane,
>   
>   	omap_overlay_update_state(priv, old_omap_state->overlay);
>   	new_omap_state->overlay = NULL;
> +
> +	if (is_omap_plane_dual_overlay(old_state)) {
> +		omap_overlay_update_state(priv, old_omap_state->r_overlay);
> +		new_omap_state->r_overlay = NULL;
> +	}
>   }
>   
>   #define FRAC_16_16(mult, div)    (((mult) << 16) / (div))
> @@ -154,6 +207,7 @@ static int omap_plane_atomic_check(struct drm_plane *plane,
>   	struct omap_plane_state *omap_state = to_omap_plane_state(new_plane_state);
>   	struct omap_global_state *omap_overlay_global_state;
>   	struct drm_crtc_state *crtc_state;
> +	bool new_r_hw_overlay = false;
>   	bool new_hw_overlay = false;
>   	u32 max_width, max_height;
>   	struct drm_crtc *crtc;
> @@ -196,7 +250,9 @@ static int omap_plane_atomic_check(struct drm_plane *plane,
>   
>   	if (!new_plane_state->visible) {
>   		omap_overlay_release(state, omap_state->overlay);
> +		omap_overlay_release(state, omap_state->r_overlay);
>   		omap_state->overlay = NULL;
> +		omap_state->r_overlay = NULL;
>   		return 0;
>   	}
>   
> @@ -213,8 +269,30 @@ static int omap_plane_atomic_check(struct drm_plane *plane,
>   	if (new_plane_state->src_h > max_height || new_plane_state->crtc_h > height)
>   		return -EINVAL;
>   
> -	if (new_plane_state->src_w > max_width || new_plane_state->crtc_w > width)
> -		return -EINVAL;
> +
> +	if (new_plane_state->src_w > max_width || new_plane_state->crtc_w > width) {
> +		bool is_fourcc_yuv = new_plane_state->fb->format->is_yuv;
> +
> +		if (is_fourcc_yuv && (((new_plane_state->src_w >> 16) / 2 & 1) ||
> +				      new_plane_state->crtc_w / 2 & 1)) {
> +			/*
> +			 * When calculating the split overlay width
> +			 * and it yield an odd value we will need to adjust
> +			 * the indivual width +/- 1. So make sure it fits
> +			 */
> +			if (new_plane_state->src_w <= ((2 * width - 1) << 16) &&
> +			    new_plane_state->crtc_w <= (2 * width - 1))
> +				new_r_hw_overlay = true;
> +			else
> +				return -EINVAL;
> +		} else {
> +			if (new_plane_state->src_w <= (2 * max_width) &&
> +			    new_plane_state->crtc_w <= (2 * width))
> +				new_r_hw_overlay = true;
> +			else
> +				return -EINVAL;
> +		}
> +	}
>   
>   	if (new_plane_state->rotation != DRM_MODE_ROTATE_0 &&
>   	    !omap_framebuffer_supports_rotation(new_plane_state->fb))
> @@ -239,24 +317,44 @@ static int omap_plane_atomic_check(struct drm_plane *plane,
>   			new_hw_overlay = true;
>   	}
>   
> +	/*
> +	 * check if we need two overlays and only have 1 or
> +	 * if we had 2 overlays but will only need 1
> +	 */
> +	if ((new_r_hw_overlay && !omap_state->r_overlay) ||
> +	    (!new_r_hw_overlay && omap_state->r_overlay))
> +		new_hw_overlay = true;

Is this 'new_hw_overlay' variable more like... "reassign_hw_overlays" 
after this patch?

>   	if (new_hw_overlay) {
>   		struct omap_hw_overlay *old_ovl = omap_state->overlay;
> +		struct omap_hw_overlay *old_r_ovl = omap_state->r_overlay;
>   		struct omap_hw_overlay *new_ovl = NULL;
> +		struct omap_hw_overlay *new_r_ovl = NULL;
>   
>   		omap_overlay_release(state, old_ovl);
> +		omap_overlay_release(state, old_r_ovl);
>   
> -		ret = omap_overlay_assign(state, plane, caps, fourcc, &new_ovl);
> +		ret = omap_overlay_assign(state, plane, caps, fourcc, &new_ovl,
> +					  new_r_hw_overlay ? &new_r_ovl : NULL);
>   		if (ret) {
>   			DBG("%s: failed to assign hw_overlay", plane->name);
>   			omap_state->overlay = NULL;
> +			omap_state->r_overlay = NULL;
>   			return ret;
>   		}
>   
>   		omap_state->overlay = new_ovl;
> +		if (new_r_hw_overlay)
> +			omap_state->r_overlay = new_r_ovl;
> +		else
> +			omap_state->r_overlay = NULL;
>   	}
>   
>   	DBG("plane: %s overlay_id: %d", plane->name, omap_state->overlay->id);
>   
> +	if (omap_state->r_overlay)
> +		DBG("plane: %s r_overlay_id: %d", plane->name, omap_state->r_overlay->id);
> +
>   	return 0;
>   }
>   
> @@ -344,6 +442,7 @@ omap_plane_atomic_duplicate_state(struct drm_plane *plane)
>   	__drm_atomic_helper_plane_duplicate_state(plane, &state->base);
>   
>   	state->overlay = current_state->overlay;
> +	state->r_overlay = current_state->r_overlay;
>   
>   	return &state->base;
>   }
> @@ -361,6 +460,15 @@ static void omap_plane_atomic_print_state(struct drm_printer *p,
>   			   omap_state->overlay->id);
>   		drm_printf(p, "\t\tcaps=0x%x\n", omap_state->overlay->caps);
>   	}
> +
> +	drm_printf(p, "\tr_overlay=%s\n", omap_state->r_overlay ?
> +					  omap_state->r_overlay->name : "(null)");
> +	if (omap_state->r_overlay) {
> +		drm_printf(p, "\t\tidx=%d\n", omap_state->r_overlay->idx);
> +		drm_printf(p, "\t\toverlay_id=%d\n",
> +			   omap_state->r_overlay->id);
> +		drm_printf(p, "\t\tcaps=0x%x\n", omap_state->r_overlay->caps);
> +	}
>   }
>   
>   static int omap_plane_atomic_set_property(struct drm_plane *plane,
> diff --git a/drivers/gpu/drm/omapdrm/omap_plane.h b/drivers/gpu/drm/omapdrm/omap_plane.h
> index 0c28fe8ffa20..a9a33e12722a 100644
> --- a/drivers/gpu/drm/omapdrm/omap_plane.h
> +++ b/drivers/gpu/drm/omapdrm/omap_plane.h
> @@ -22,5 +22,6 @@ struct drm_plane *omap_plane_init(struct drm_device *dev,
>   		u32 possible_crtcs);
>   void omap_plane_install_properties(struct drm_plane *plane,
>   		struct drm_mode_object *obj);
> +bool is_omap_plane_dual_overlay(struct drm_plane_state *state);
>   
>   #endif /* __OMAPDRM_PLANE_H__ */
> 

Re: [PATCH v6 1/9] drm/omap: add sanity plane state check

[Neil Armstrong]

On 27/10/2021 10:30, Tomi Valkeinen wrote:
> On 27/10/2021 11:29, Tomi Valkeinen wrote:
>> On 18/10/2021 17:28, Neil Armstrong wrote:
>>> Call drm_atomic_helper_check_plane_state() from the plane
>>> atomic_check() callback in order to add plane state sanity
>>> checking.
>>>
>>> It will permit filtering out totally bad scaling factors, even
>>> if the real check are done later in the atomic commit.
>>
>> I think there's more to it: the function sets plane_state->visible, which is used in later patches.
> 
> Maybe that could be mentioned in a comment. But otherwise:
> 
> Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>

Thanks, I'll update the comment to mention this.

Neil

> 
>  Tomi

Re: [PATCH v6 1/9] drm/omap: add sanity plane state check

[Neil Armstrong]

On 27/10/2021 10:30, Tomi Valkeinen wrote:
> On 27/10/2021 11:29, Tomi Valkeinen wrote:
>> On 18/10/2021 17:28, Neil Armstrong wrote:
>>> Call drm_atomic_helper_check_plane_state() from the plane
>>> atomic_check() callback in order to add plane state sanity
>>> checking.
>>>
>>> It will permit filtering out totally bad scaling factors, even
>>> if the real check are done later in the atomic commit.
>>
>> I think there's more to it: the function sets plane_state->visible, which is used in later patches.
> 
> Maybe that could be mentioned in a comment. But otherwise:
> 
> Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>

Thanks, I'll update the comment to mention this.

Neil

> 
>  Tomi

Re: [PATCH v6 9/9] drm/omap: Add a 'right overlay' to plane state

[Neil Armstrong]

Hi,

On 27/10/2021 14:50, Tomi Valkeinen wrote:
> On 18/10/2021 17:28, Neil Armstrong wrote:
>> From: Benoit Parrot <bparrot@ti.com>
>>
>> If the drm_plane has a source width that's greater than the max width
>> supported by a single hw overlay, then we assign a 'r_overlay' to it in
>> omap_plane_atomic_check().
>>
>> Both overlays should have the capabilities required to handle the source
>> framebuffer. The only parameters that vary between the left and right
>> hwoverlays are the src_w, crtc_w, src_x and crtc_x as we just even chop
>> the fb into left and right halves.
>>
>> We also take care of not creating odd width size when dealing with YUV
>> formats.
>>
>> Since both halves need to be 'appear' side by side the zpos is
>> recalculated when dealing with dual overlay cases so that the other
>> planes zpos is consistent.
>>
>> Depending on user space usage it is possible that on occasion the number
>> of requested planes exceeds the numbers of overlays required to display
>> them. In that case a failure would be returned for the plane that cannot
>> be handled at that time. It is up to user space to make sure the H/W
>> resource are not over-subscribed.
>>
>> Signed-off-by: Benoit Parrot <bparrot@ti.com>
>> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
>> ---
>>   drivers/gpu/drm/omapdrm/omap_drv.c     |  91 ++++++++++++++++++-
>>   drivers/gpu/drm/omapdrm/omap_fb.c      |  33 ++++++-
>>   drivers/gpu/drm/omapdrm/omap_fb.h      |   4 +-
>>   drivers/gpu/drm/omapdrm/omap_overlay.c |  23 ++++-
>>   drivers/gpu/drm/omapdrm/omap_overlay.h |   3 +-
>>   drivers/gpu/drm/omapdrm/omap_plane.c   | 120 +++++++++++++++++++++++--
>>   drivers/gpu/drm/omapdrm/omap_plane.h   |   1 +
>>   7 files changed, 263 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c
>> index c7912374d393..f088b6313950 100644
>> --- a/drivers/gpu/drm/omapdrm/omap_drv.c
>> +++ b/drivers/gpu/drm/omapdrm/omap_drv.c
>> @@ -117,6 +117,95 @@ static void omap_atomic_commit_tail(struct drm_atomic_state *old_state)
>>       dispc_runtime_put(priv->dispc);
>>   }
>>   +static int drm_atomic_state_normalized_zpos_cmp(const void *a, const void *b)
>> +{
>> +    const struct drm_plane_state *sa = *(struct drm_plane_state **)a;
>> +    const struct drm_plane_state *sb = *(struct drm_plane_state **)b;
>> +
>> +    if (sa->normalized_zpos != sb->normalized_zpos)
>> +        return sa->normalized_zpos - sb->normalized_zpos;
>> +    else
>> +        return sa->plane->base.id - sb->plane->base.id;
>> +}
> 
> I think this, or the function below, needs a comment. I don't think it's obvious what's the logic here. It's mostly a copy of drm_atomic_normalize_zpos and drm_atomic_helper_crtc_normalize_zpos, if I'm not mistaken, it migth be good to point out what the difference is.

It's explained in the commit message:
>> Since both halves need to be 'appear' side by side the zpos is
>> recalculated when dealing with dual overlay cases so that the other
>> planes zpos is consistent.

Not sure what to add more, should I add it as comment in the code ?

> 
> I'm also wondering if these normalize_zpos functions should be split to a separate patch (without the is_omap_plane_dual_overlay call part).

It's tied to the introduction of 'right overlay', impossible to implement it without the
dual overlays functions and variables.

> 
>> +static int omap_atomic_update_normalize_zpos(struct drm_device *dev,
>> +                         struct drm_atomic_state *state)
>> +{
>> +    struct drm_crtc *crtc;
>> +    struct drm_crtc_state *old_state, *new_state;
>> +    struct drm_plane *plane;
>> +    int c, i, n, inc;
>> +    int total_planes = dev->mode_config.num_total_plane;
>> +    struct drm_plane_state **states;
>> +    int ret = 0;
>> +
>> +    states = kmalloc_array(total_planes, sizeof(*states), GFP_KERNEL);
>> +    if (!states)
>> +        return -ENOMEM;
>> +
>> +    for_each_oldnew_crtc_in_state(state, crtc, old_state, new_state, c) {
>> +        if (old_state->plane_mask == new_state->plane_mask &&
>> +            !new_state->zpos_changed)
>> +            continue;
>> +
>> +        /* Reset plane increment and index value for every crtc */
>> +        n = 0;
>> +
>> +        /*
>> +         * Normalization process might create new states for planes
>> +         * which normalized_zpos has to be recalculated.
>> +         */
>> +        drm_for_each_plane_mask(plane, dev, new_state->plane_mask) {
>> +            struct drm_plane_state *plane_state =
>> +                drm_atomic_get_plane_state(new_state->state,
>> +                               plane);
>> +            if (IS_ERR(plane_state)) {
>> +                ret = PTR_ERR(plane_state);
>> +                goto done;
>> +            }
>> +            states[n++] = plane_state;
>> +        }
>> +
>> +        sort(states, n, sizeof(*states),
>> +             drm_atomic_state_normalized_zpos_cmp, NULL);
>> +
>> +        for (i = 0, inc = 0; i < n; i++) {
>> +            plane = states[i]->plane;
>> +
>> +            states[i]->normalized_zpos = i + inc;
>> +            DRM_DEBUG_ATOMIC("[PLANE:%d:%s] updated normalized zpos value %d\n",
>> +                     plane->base.id, plane->name,
>> +                     states[i]->normalized_zpos);
>> +
>> +            if (is_omap_plane_dual_overlay(states[i]))
>> +                inc++;
>> +        }
>> +        new_state->zpos_changed = true;
>> +    }
>> +
>> +done:
>> +    kfree(states);
>> +    return ret;
>> +}
>> +
>> +static int omap_atomic_check(struct drm_device *dev,
>> +                 struct drm_atomic_state *state)
>> +{
>> +    int ret;
>> +
>> +    ret = drm_atomic_helper_check(dev, state);
>> +    if (ret)
>> +        return ret;
>> +
>> +    if (dev->mode_config.normalize_zpos) {
>> +        ret = omap_atomic_update_normalize_zpos(dev, state);
>> +        if (ret)
>> +            return ret;
>> +    }
>> +
>> +    return 0;
>> +}
>> +

[...]

Thanks,
Neil

Re: [PATCH v6 9/9] drm/omap: Add a 'right overlay' to plane state

[Neil Armstrong]

Hi,

On 27/10/2021 14:50, Tomi Valkeinen wrote:
> On 18/10/2021 17:28, Neil Armstrong wrote:
>> From: Benoit Parrot <bparrot@ti.com>
>>
>> If the drm_plane has a source width that's greater than the max width
>> supported by a single hw overlay, then we assign a 'r_overlay' to it in
>> omap_plane_atomic_check().
>>
>> Both overlays should have the capabilities required to handle the source
>> framebuffer. The only parameters that vary between the left and right
>> hwoverlays are the src_w, crtc_w, src_x and crtc_x as we just even chop
>> the fb into left and right halves.
>>
>> We also take care of not creating odd width size when dealing with YUV
>> formats.
>>
>> Since both halves need to be 'appear' side by side the zpos is
>> recalculated when dealing with dual overlay cases so that the other
>> planes zpos is consistent.
>>
>> Depending on user space usage it is possible that on occasion the number
>> of requested planes exceeds the numbers of overlays required to display
>> them. In that case a failure would be returned for the plane that cannot
>> be handled at that time. It is up to user space to make sure the H/W
>> resource are not over-subscribed.
>>
>> Signed-off-by: Benoit Parrot <bparrot@ti.com>
>> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
>> ---
>>   drivers/gpu/drm/omapdrm/omap_drv.c     |  91 ++++++++++++++++++-
>>   drivers/gpu/drm/omapdrm/omap_fb.c      |  33 ++++++-
>>   drivers/gpu/drm/omapdrm/omap_fb.h      |   4 +-
>>   drivers/gpu/drm/omapdrm/omap_overlay.c |  23 ++++-
>>   drivers/gpu/drm/omapdrm/omap_overlay.h |   3 +-
>>   drivers/gpu/drm/omapdrm/omap_plane.c   | 120 +++++++++++++++++++++++--
>>   drivers/gpu/drm/omapdrm/omap_plane.h   |   1 +
>>   7 files changed, 263 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c
>> index c7912374d393..f088b6313950 100644
>> --- a/drivers/gpu/drm/omapdrm/omap_drv.c
>> +++ b/drivers/gpu/drm/omapdrm/omap_drv.c
>> @@ -117,6 +117,95 @@ static void omap_atomic_commit_tail(struct drm_atomic_state *old_state)
>>       dispc_runtime_put(priv->dispc);
>>   }
>>   +static int drm_atomic_state_normalized_zpos_cmp(const void *a, const void *b)
>> +{
>> +    const struct drm_plane_state *sa = *(struct drm_plane_state **)a;
>> +    const struct drm_plane_state *sb = *(struct drm_plane_state **)b;
>> +
>> +    if (sa->normalized_zpos != sb->normalized_zpos)
>> +        return sa->normalized_zpos - sb->normalized_zpos;
>> +    else
>> +        return sa->plane->base.id - sb->plane->base.id;
>> +}
> 
> I think this, or the function below, needs a comment. I don't think it's obvious what's the logic here. It's mostly a copy of drm_atomic_normalize_zpos and drm_atomic_helper_crtc_normalize_zpos, if I'm not mistaken, it migth be good to point out what the difference is.

It's explained in the commit message:
>> Since both halves need to be 'appear' side by side the zpos is
>> recalculated when dealing with dual overlay cases so that the other
>> planes zpos is consistent.

Not sure what to add more, should I add it as comment in the code ?

> 
> I'm also wondering if these normalize_zpos functions should be split to a separate patch (without the is_omap_plane_dual_overlay call part).

It's tied to the introduction of 'right overlay', impossible to implement it without the
dual overlays functions and variables.

> 
>> +static int omap_atomic_update_normalize_zpos(struct drm_device *dev,
>> +                         struct drm_atomic_state *state)
>> +{
>> +    struct drm_crtc *crtc;
>> +    struct drm_crtc_state *old_state, *new_state;
>> +    struct drm_plane *plane;
>> +    int c, i, n, inc;
>> +    int total_planes = dev->mode_config.num_total_plane;
>> +    struct drm_plane_state **states;
>> +    int ret = 0;
>> +
>> +    states = kmalloc_array(total_planes, sizeof(*states), GFP_KERNEL);
>> +    if (!states)
>> +        return -ENOMEM;
>> +
>> +    for_each_oldnew_crtc_in_state(state, crtc, old_state, new_state, c) {
>> +        if (old_state->plane_mask == new_state->plane_mask &&
>> +            !new_state->zpos_changed)
>> +            continue;
>> +
>> +        /* Reset plane increment and index value for every crtc */
>> +        n = 0;
>> +
>> +        /*
>> +         * Normalization process might create new states for planes
>> +         * which normalized_zpos has to be recalculated.
>> +         */
>> +        drm_for_each_plane_mask(plane, dev, new_state->plane_mask) {
>> +            struct drm_plane_state *plane_state =
>> +                drm_atomic_get_plane_state(new_state->state,
>> +                               plane);
>> +            if (IS_ERR(plane_state)) {
>> +                ret = PTR_ERR(plane_state);
>> +                goto done;
>> +            }
>> +            states[n++] = plane_state;
>> +        }
>> +
>> +        sort(states, n, sizeof(*states),
>> +             drm_atomic_state_normalized_zpos_cmp, NULL);
>> +
>> +        for (i = 0, inc = 0; i < n; i++) {
>> +            plane = states[i]->plane;
>> +
>> +            states[i]->normalized_zpos = i + inc;
>> +            DRM_DEBUG_ATOMIC("[PLANE:%d:%s] updated normalized zpos value %d\n",
>> +                     plane->base.id, plane->name,
>> +                     states[i]->normalized_zpos);
>> +
>> +            if (is_omap_plane_dual_overlay(states[i]))
>> +                inc++;
>> +        }
>> +        new_state->zpos_changed = true;
>> +    }
>> +
>> +done:
>> +    kfree(states);
>> +    return ret;
>> +}
>> +
>> +static int omap_atomic_check(struct drm_device *dev,
>> +                 struct drm_atomic_state *state)
>> +{
>> +    int ret;
>> +
>> +    ret = drm_atomic_helper_check(dev, state);
>> +    if (ret)
>> +        return ret;
>> +
>> +    if (dev->mode_config.normalize_zpos) {
>> +        ret = omap_atomic_update_normalize_zpos(dev, state);
>> +        if (ret)
>> +            return ret;
>> +    }
>> +
>> +    return 0;
>> +}
>> +

[...]

Thanks,
Neil