[RFC PATCH 0/3] Add display sharing support in tidss

[RFC PATCH 0/3] Add display sharing support in tidss

[Devarsh Thakkar]

This adds display sharing support in tidss display driver along with an
example overlay devicetree file using which can be used to enable display
sharing on AM62x devices with device manager core i.e. R5F expected to run
a custom firmware which supports corresponding display sharing
configuration.

As resources can be partitioned at different levels a flexible scheme is
followed while designing devicetree bindings and driver changes keeping in
mind possible scenarios in which resources can be partitioned and various
DSS hardware IP's supported across different devices.

A rebased version of this patch has been tested on AM62P SoC which also
supports same DSS IP as AM62x and the patch with display sharing
functionality is already available in vendor-specific kernel source tree
[1] along with documentation which explains the design [2] and DM firmware
support [3].

NOTE1: This is marked as RFC for upstream since AM62P is not yet supported
in upstream tree and for AM62x SoC which is the target SoC for this patch,
the dss sharing functionality is not validated on upstream tree due to
missing OLDI support and display sharing DM firmware support, but we still
wanted to get some feedback on this.

NOTE2: This series depends on :
https://lore.kernel.org/all/20240115125716.560363-1-devarsht@ti.com/

[1] :
https://git.ti.com/cgit/ti-linux-kernel/ti-linux-kernel/commit/?h=09.01.00.007&id=d805270609cfb6b2e67bd2fd5959d71f48393196 
https://git.ti.com/cgit/ti-linux-kernel/ti-linux-kernel/commit/?h=09.01.00.007&id=93d751a94cbf9ad07c7f658e78aa510d919d7cd6
https://git.ti.com/cgit/ti-linux-kernel/ti-linux-kernel/commit/?h=09.01.00.007&id=665c17837dc8bed27e8d63388f8f7f7a85c0cd94
https://git.ti.com/cgit/ti-linux-kernel/ti-linux-kernel/commit/?h=ti-linux-6.1.y-cicd&id=f8d7f1a9617862af922c6bc10e0765ba4f4857d6
 
[2] :
https://software-dl.ti.com/processor-sdk-linux/esd/AM62PX/09_01_00_08/exports/docs/linux/Foundational_Components/Kernel/Kernel_Drivers/Display/DSS7.html#driver-features
(Display Sharing mode feature description)
https://software-dl.ti.com/processor-sdk-linux/esd/AM62PX/09_01_00_08/exports/docs/linux/How_to_Guides/Target/How_to_enable_display_sharing_between_remotecore_and_Linux.html
(How To Guide)
https://software-dl.ti.com/processor-sdk-linux/esd/AM62PX/09_01_00_08/exports/docs/system/Demo_User_Guides/Display_Cluster_User_Guide.html
(Display Cluster user guide with demo details)

[3] :
https://git.ti.com/cgit/processor-firmware/ti-linux-firmware/tree/ti-dm/am62pxx/dss_display_share.wkup-r5f0_0.release.strip.out?h=ti-linux-firmware-next

Devarsh Thakkar (3):
  dt-bindings: display: ti,am65x-dss: Add support for display sharing
    mode
  drm/tidss: Add support for display sharing
  arm64: dts: ti: k3-am62x: Add overlay to use DSS in display sharing
    mode

 .../bindings/display/ti/ti,am65x-dss.yaml     |  82 ++++++
 arch/arm64/boot/dts/ti/Makefile               |   1 +
 .../dts/ti/k3-am62x-sk-dss-shared-mode.dtso   |  23 ++
 drivers/gpu/drm/tidss/tidss_crtc.c            | 120 ++++++++-
 drivers/gpu/drm/tidss/tidss_dispc.c           | 254 ++++++++++++++++--
 drivers/gpu/drm/tidss/tidss_dispc.h           |   2 +-
 drivers/gpu/drm/tidss/tidss_drv.c             |  33 ++-
 drivers/gpu/drm/tidss/tidss_drv.h             |   6 +
 8 files changed, 481 insertions(+), 40 deletions(-)
 create mode 100644 arch/arm64/boot/dts/ti/k3-am62x-sk-dss-shared-mode.dtso

-- 
2.34.1

[RFC PATCH 1/3] dt-bindings: display: ti, am65x-dss: Add support for display sharing mode

[Devarsh Thakkar]

Add support for using TI Keystone DSS hardware present in display
sharing mode.

TI Keystone DSS hardware supports partitioning of resources between
multiple hosts as it provides separate register space and unique
interrupt line to each host.

The DSS hardware can be used in shared mode in such a way that one or
more of video planes can be owned by Linux wherease other planes can be
owned by remote cores.

One or more of the video ports can be dedicated exclusively to a
processing core, wherease some of the video ports can be shared between
two hosts too with only one of them having write access.

Signed-off-by: Devarsh Thakkar <devarsht@ti.com>
---
 .../bindings/display/ti/ti,am65x-dss.yaml     | 82 +++++++++++++++++++
 1 file changed, 82 insertions(+)

diff --git a/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml b/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml
index 55e3e490d0e6..d9bc69fbf1fb 100644
--- a/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml
+++ b/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml
@@ -112,6 +112,86 @@ properties:
       Input memory (from main memory to dispc) bandwidth limit in
       bytes per second
 
+  ti,dss-shared-mode:
+    type: boolean
+    description:
+      TI DSS7 supports sharing of display between multiple hosts
+      as it provides separate register space for display configuration and
+      unique interrupt line to each host.
+      One of the host is provided access to the global display
+      configuration labelled as "common" region of DSS allows that host
+      exclusive access to global registers of DSS while other host can
+      configure the display for it's usage using a separate register
+      space labelled as "common1".
+      The DSS resources can be partitioned in such a way that one or more
+      of the video planes are owned by Linux whereas other video planes
+      can be owned by a remote core.
+      The video port controlling these planes acts as a shared video port
+      and it can be configured with write access either by Linux or the
+      remote core in which case Linux only has read-only access to that
+      video port.
+
+  ti,dss-shared-mode-planes:
+    description:
+      The video layer that is owned by processing core running Linux.
+      The display driver running from Linux has exclusive write access to
+      this video layer.
+    $ref: /schemas/types.yaml#/definitions/string
+    enum: [vidl, vid]
+
+  ti,dss-shared-mode-vp:
+    description:
+      The video port that is being used in context of processing core
+      running Linux with display susbsytem being used in shared mode.
+      This can be owned either by the processing core running Linux in
+      which case Linux has the write access and the responsibility to
+      configure this video port and the associated overlay manager or
+      it can be shared between core running Linux and a remote core
+      with remote core provided with write access to this video port and
+      associated overlay managers and remote core configures and drives
+      this video port also feeding data from one or more of the
+      video planes owned by Linux, with Linux only having read-only access
+      to this video port and associated overlay managers.
+
+    $ref: /schemas/types.yaml#/definitions/string
+    enum: [vp1, vp2]
+
+  ti,dss-shared-mode-common:
+    description:
+      The DSS register region owned by processing core running Linux.
+    $ref: /schemas/types.yaml#/definitions/string
+    enum: [common, common1]
+
+  ti,dss-shared-mode-vp-owned:
+    description:
+      This tells whether processing core running Linux has write access to
+      the video ports enlisted in ti,dss-shared-mode-vps.
+    $ref: /schemas/types.yaml#/definitions/uint32
+    enum: [0, 1]
+
+  ti,dss-shared-mode-plane-zorder:
+    description:
+      The zorder of the planes owned by Linux.
+      For the scenario where Linux is not having write access to associated
+      video port, this field is just for
+      informational purpose to enumerate the zorder configuration
+      being used by remote core.
+
+    $ref: /schemas/types.yaml#/definitions/uint32
+    enum: [0, 1]
+
+dependencies:
+  ti,dss-shared-mode: [ 'ti,dss-shared-mode-planes', 'ti,dss-shared-mode-vp',
+                        'ti,dss-shared-mode-plane-zorder', 'ti,dss-shared-mode-vp-owned']
+  ti,dss-shared-mode-vp: ['ti,dss-shared-mode', 'ti,dss-shared-mode-planes',
+                          'ti,dss-shared-mode-plane-zorder', 'ti,dss-shared-mode-vp-owned']
+  ti,dss-shared-mode-planes: ['ti,dss-shared-mode', 'ti,dss-shared-mode-vp',
+                              'ti,dss-shared-mode-plane-zorder', 'ti,dss-shared-mode-vp-owned']
+  ti,dss-shared-mode-plane-zorder: ['ti,dss-shared-mode-planes', 'ti,dss-shared-mode-vp',
+                                    'ti,dss-shared-mode', 'ti,dss-shared-mode-vp-owned']
+  ti,dss-shared-mode-vp-owned: ['ti,dss-shared-mode-planes', 'ti,dss-shared-mode-vp',
+                                'ti,dss-shared-mode', 'ti,dss-shared-mode-plane-zorder']
+
 allOf:
   - if:
       properties:
@@ -123,6 +203,8 @@ allOf:
         ports:
           properties:
             port@0: false
+            ti,dss-shared-mode-vp:
+            enum: [vp2]
 
 required:
   - compatible
-- 
2.34.1

[RFC PATCH 2/3] drm/tidss: Add support for display sharing

[Devarsh Thakkar]

Display subsystem present in TI Keystone family of devices supports sharing
of display between multiple hosts as it provides separate register space
(common* region) for each host to programming display controller and also a
unique interrupt line for each host.

This adds support for display sharing, by allowing partitioning of
resources either at video port level or at video plane level as
described below :

1) Linux can own (i.e have write access) completely one or more of video
ports along with corresponding resources (viz. overlay managers,
video planes) used by Linux in context of those video ports.
Even if Linux is owning
these video ports it can still share this video port with a remote core
which can own one or more video planes associated with this video port.

2) Linux owns one or more of the video planes with video port
(along with corresponding overlay manager) associated with these planes
being owned and controlled by a remote core. Linux still has read-only
access to the associated video port and overlay managers so that it can
parse the settings made by remote core.

For both the cases, the resources used in context of processing core
running Linux along with ownership information are exposed by user as
part of device-tree blob and driver uses an updated feature list tailored
for this shared mode accordingly. The driver also auto-populates
matching overlay managers and output types from shared video
port list provided in device-tree blob.
In dispc_feature struct remove const access specfier for output_type
array as it is required to be updated dynamically in run-time for shared
mode.

For 2) where Linux is only owning a set of video planes with
corresponding video port and overlay manager controlled by a remote
core, separate set of CRTC callbacks are used which just latch on
to the preset mode set by remote core, thus avoiding any reconfiguration
of associated video ports, overlay managers and clocks.
For this case, it is also checked that Linux controlled video planes
don't exceed screen size set by remote core while running the display.
Display clocks and OLDI related fields are also not
populated for this scenario as remote core is owning those resources.

For 1), where Linux owns only a set of video port and associated
planes with rest of resources owned completely by remote cores,
only those set of resources are exposed to Linux and programmed using
traditional CRTC helpers and rest of video ports and associated resources
are removed from feature list accordingly.

Signed-off-by: Devarsh Thakkar <devarsht@ti.com>
---
 drivers/gpu/drm/tidss/tidss_crtc.c  | 120 ++++++++++++-
 drivers/gpu/drm/tidss/tidss_dispc.c | 254 +++++++++++++++++++++++++---
 drivers/gpu/drm/tidss/tidss_dispc.h |   2 +-
 drivers/gpu/drm/tidss/tidss_drv.c   |  33 ++--
 drivers/gpu/drm/tidss/tidss_drv.h   |   6 +
 5 files changed, 375 insertions(+), 40 deletions(-)

diff --git a/drivers/gpu/drm/tidss/tidss_crtc.c b/drivers/gpu/drm/tidss/tidss_crtc.c
index 5f838980c7a1..f6a877ff4c6c 100644
--- a/drivers/gpu/drm/tidss/tidss_crtc.c
+++ b/drivers/gpu/drm/tidss/tidss_crtc.c
@@ -31,13 +31,19 @@ static void tidss_crtc_finish_page_flip(struct tidss_crtc *tcrtc)
 	/*
 	 * New settings are taken into use at VFP, and GO bit is cleared at
 	 * the same time. This happens before the vertical blank interrupt.
-	 * So there is a small change that the driver sets GO bit after VFP, but
+	 * So there is a small chance that the driver sets GO bit after VFP, but
 	 * before vblank, and we have to check for that case here.
+	 *
+	 * For a video port shared between Linux and remote core but owned by remote core,
+	 * this is not required since Linux just attaches to mode that was preset by remote
+	 * core with which display is being shared.
 	 */
-	busy = dispc_vp_go_busy(tidss->dispc, tcrtc->hw_videoport);
-	if (busy) {
-		spin_unlock_irqrestore(&ddev->event_lock, flags);
-		return;
+	if (!tidss->shared_mode || tidss->shared_mode_owned_vps[tcrtc->hw_videoport]) {
+		busy = dispc_vp_go_busy(tidss->dispc, tcrtc->hw_videoport);
+		if (busy) {
+			spin_unlock_irqrestore(&ddev->event_lock, flags);
+			return;
+		}
 	}
 
 	event = tcrtc->event;
@@ -208,6 +214,44 @@ static void tidss_crtc_atomic_flush(struct drm_crtc *crtc,
 	spin_unlock_irqrestore(&ddev->event_lock, flags);
 }
 
+static void tidss_shared_vp_crtc_atomic_flush(struct drm_crtc *crtc,
+					      struct drm_atomic_state *state)
+{
+	struct tidss_crtc *tcrtc = to_tidss_crtc(crtc);
+	struct drm_device *ddev = crtc->dev;
+	unsigned long flags;
+
+	dev_dbg(ddev->dev,
+		"%s: %s enabled %d, needs modeset %d, event %p\n", __func__,
+		crtc->name, drm_atomic_crtc_needs_modeset(crtc->state),
+		crtc->state->enable, crtc->state->event);
+
+	/* There is nothing to do if CRTC is not going to be enabled. */
+	if (!crtc->state->enable)
+		return;
+	/*
+	 * Flush CRTC changes with go bit only if new modeset is not
+	 * coming, so CRTC is enabled trough out the commit.
+	 */
+	if (drm_atomic_crtc_needs_modeset(crtc->state))
+		return;
+
+	/* We should have event if CRTC is enabled through out this commit. */
+	if (WARN_ON(!crtc->state->event))
+		return;
+
+	WARN_ON(drm_crtc_vblank_get(crtc) != 0);
+
+	spin_lock_irqsave(&ddev->event_lock, flags);
+
+	WARN_ON(tcrtc->event);
+
+	tcrtc->event = crtc->state->event;
+	crtc->state->event = NULL;
+
+	spin_unlock_irqrestore(&ddev->event_lock, flags);
+}
+
 static void tidss_crtc_atomic_enable(struct drm_crtc *crtc,
 				     struct drm_atomic_state *state)
 {
@@ -253,6 +297,27 @@ static void tidss_crtc_atomic_enable(struct drm_crtc *crtc,
 	spin_unlock_irqrestore(&ddev->event_lock, flags);
 }
 
+static void tidss_shared_vp_crtc_atomic_enable(struct drm_crtc *crtc,
+					       struct drm_atomic_state *state)
+{
+	struct drm_device *ddev = crtc->dev;
+	unsigned long flags;
+
+	dev_dbg(ddev->dev, "%s, event %p\n", __func__, crtc->state->event);
+
+	/* Turn vertical blanking interrupt reporting on. */
+	drm_crtc_vblank_on(crtc);
+
+	spin_lock_irqsave(&ddev->event_lock, flags);
+
+	if (crtc->state->event) {
+		drm_crtc_send_vblank_event(crtc, crtc->state->event);
+		crtc->state->event = NULL;
+	}
+
+	spin_unlock_irqrestore(&ddev->event_lock, flags);
+}
+
 static void tidss_crtc_atomic_disable(struct drm_crtc *crtc,
 				      struct drm_atomic_state *state)
 {
@@ -288,6 +353,27 @@ static void tidss_crtc_atomic_disable(struct drm_crtc *crtc,
 	tidss_runtime_put(tidss);
 }
 
+static void tidss_shared_vp_crtc_atomic_disable(struct drm_crtc *crtc,
+						struct drm_atomic_state *state)
+{
+	struct tidss_crtc *tcrtc = to_tidss_crtc(crtc);
+	struct drm_device *ddev = crtc->dev;
+	unsigned long flags;
+
+	dev_dbg(ddev->dev, "%s, event %p\n", __func__, crtc->state->event);
+
+	reinit_completion(&tcrtc->framedone_completion);
+
+	spin_lock_irqsave(&ddev->event_lock, flags);
+	if (crtc->state->event) {
+		drm_crtc_send_vblank_event(crtc, crtc->state->event);
+		crtc->state->event = NULL;
+	}
+	spin_unlock_irqrestore(&ddev->event_lock, flags);
+
+	drm_crtc_vblank_off(crtc);
+}
+
 static
 enum drm_mode_status tidss_crtc_mode_valid(struct drm_crtc *crtc,
 					   const struct drm_display_mode *mode)
@@ -304,7 +390,14 @@ static const struct drm_crtc_helper_funcs tidss_crtc_helper_funcs = {
 	.atomic_flush = tidss_crtc_atomic_flush,
 	.atomic_enable = tidss_crtc_atomic_enable,
 	.atomic_disable = tidss_crtc_atomic_disable,
+	.mode_valid = tidss_crtc_mode_valid,
+};
 
+static const struct drm_crtc_helper_funcs tidss_shared_vp_crtc_helper_funcs = {
+	.atomic_check = tidss_crtc_atomic_check,
+	.atomic_flush = tidss_shared_vp_crtc_atomic_flush,
+	.atomic_enable = tidss_shared_vp_crtc_atomic_enable,
+	.atomic_disable = tidss_shared_vp_crtc_atomic_disable,
 	.mode_valid = tidss_crtc_mode_valid,
 };
 
@@ -406,6 +499,10 @@ struct tidss_crtc *tidss_crtc_create(struct tidss_device *tidss,
 	bool has_ctm = tidss->feat->vp_feat.color.has_ctm;
 	int ret;
 
+	dev_dbg(tidss->dev, "%s: tidss->shared_mode: %d tidss->shared_mode_owned_vps[%d] = %d\n",
+		__func__, tidss->shared_mode, hw_videoport,
+		tidss->shared_mode_owned_vps[hw_videoport]);
+
 	tcrtc = kzalloc(sizeof(*tcrtc), GFP_KERNEL);
 	if (!tcrtc)
 		return ERR_PTR(-ENOMEM);
@@ -422,8 +519,17 @@ struct tidss_crtc *tidss_crtc_create(struct tidss_device *tidss,
 		return ERR_PTR(ret);
 	}
 
-	drm_crtc_helper_add(crtc, &tidss_crtc_helper_funcs);
-
+	/* Use shared video port CRTC helpers which don't access associated
+	 * video port and overlay register spaces when Linux is not owning the
+	 * video port.
+	 */
+	if (tidss->shared_mode && !tidss->shared_mode_owned_vps[hw_videoport]) {
+		drm_crtc_helper_add(crtc, &tidss_shared_vp_crtc_helper_funcs);
+		dev_dbg(tidss->dev, "%s: vp%d is being shared with Linux\n", __func__,
+			hw_videoport + 1);
+	} else {
+		drm_crtc_helper_add(crtc, &tidss_crtc_helper_funcs);
+	}
 	/*
 	 * The dispc gamma functions adapt to what ever size we ask
 	 * from it no matter what HW supports. X-server assumes 256
diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c
index 1ad711f8d2a8..35a82198629f 100644
--- a/drivers/gpu/drm/tidss/tidss_dispc.c
+++ b/drivers/gpu/drm/tidss/tidss_dispc.c
@@ -273,6 +273,18 @@ const struct dispc_features dispc_j721e_feats = {
 	.vid_order = { 1, 3, 0, 2 },
 };
 
+static const u16 tidss_am62_common1_regs[DISPC_COMMON_REG_TABLE_LEN] = {
+	[DISPC_IRQ_EOI_OFF] =			0x24,
+	[DISPC_IRQSTATUS_RAW_OFF] =		0x28,
+	[DISPC_IRQSTATUS_OFF] =			0x2c,
+	[DISPC_IRQENABLE_SET_OFF] =		0x30,
+	[DISPC_IRQENABLE_CLR_OFF] =		0x40,
+	[DISPC_VID_IRQENABLE_OFF] =		0x44,
+	[DISPC_VID_IRQSTATUS_OFF] =		0x58,
+	[DISPC_VP_IRQENABLE_OFF] =		0x70,
+	[DISPC_VP_IRQSTATUS_OFF] =		0x7c,
+};
+
 const struct dispc_features dispc_am625_feats = {
 	.max_pclk_khz = {
 		[DISPC_VP_DPI] = 165000,
@@ -1228,6 +1240,22 @@ enum drm_mode_status dispc_vp_mode_valid(struct dispc_device *dispc,
 
 	max_pclk = dispc->feat->max_pclk_khz[bus_type];
 
+	/*
+	 * For shared mode, with remote core driving the video port, make sure that Linux
+	 * controlled primary plane doesn't exceed video port screen size set by remote core
+	 */
+	if (dispc->tidss->shared_mode && !dispc->tidss->shared_mode_owned_vps[hw_videoport]) {
+		int vp_hdisplay = VP_REG_GET(dispc, hw_videoport, DISPC_VP_SIZE_SCREEN, 11, 0) + 1;
+		int vp_vdisplay = VP_REG_GET(dispc, hw_videoport, DISPC_VP_SIZE_SCREEN, 27, 16) + 1;
+
+		if (mode->hdisplay > vp_hdisplay ||
+		    mode->vdisplay > vp_vdisplay) {
+			dev_err(dispc->dev, "%dx%d exceeds VP screen size %dx%d in shared mode\n",
+				mode->hdisplay, mode->vdisplay, vp_hdisplay, vp_vdisplay);
+			return MODE_BAD;
+		}
+	}
+
 	if (WARN_ON(max_pclk == 0))
 		return MODE_BAD;
 
@@ -1276,15 +1304,18 @@ enum drm_mode_status dispc_vp_mode_valid(struct dispc_device *dispc,
 		return MODE_BAD_VVALUE;
 
 	if (dispc->memory_bandwidth_limit) {
-		const unsigned int bpp = 4;
-		u64 bandwidth;
+		if (!dispc->tidss->shared_mode ||
+		    dispc->tidss->shared_mode_owned_vps[hw_videoport]) {
+			const unsigned int bpp = 4;
+			u64 bandwidth;
 
-		bandwidth = 1000 * mode->clock;
-		bandwidth = bandwidth * mode->hdisplay * mode->vdisplay * bpp;
-		bandwidth = div_u64(bandwidth, mode->htotal * mode->vtotal);
+			bandwidth = 1000 * mode->clock;
+			bandwidth = bandwidth * mode->hdisplay * mode->vdisplay * bpp;
+			bandwidth = div_u64(bandwidth, mode->htotal * mode->vtotal);
 
-		if (dispc->memory_bandwidth_limit < bandwidth)
-			return MODE_BAD;
+			if (dispc->memory_bandwidth_limit < bandwidth)
+				return MODE_BAD;
+		}
 	}
 
 	return MODE_OK;
@@ -2655,6 +2686,147 @@ static void dispc_vp_set_color_mgmt(struct dispc_device *dispc,
 		dispc_k3_vp_set_ctm(dispc, hw_videoport, ctm);
 }
 
+static int get_vp_idx_from_vp(const char *vp_name)
+{
+	u32 vp_idx;
+
+	if (!strcmp("vp1", vp_name))
+		vp_idx = 0;
+	else if (!strcmp("vp2", vp_name))
+		vp_idx = 1;
+	else if (!strcmp("vp3", vp_name))
+		vp_idx = 2;
+	else if (!strcmp("vp4", vp_name))
+		vp_idx = 3;
+	else
+		return 0;
+
+	return vp_idx;
+}
+
+static const char *get_ovr_from_vp(const char *vp_name)
+{
+	const char *ovr_name = NULL;
+
+	if (!strcmp("vp1", vp_name))
+		ovr_name = "ovr1";
+	else if (!strcmp("vp2", vp_name))
+		ovr_name = "ovr2";
+	else if (!strcmp("vp3", vp_name))
+		ovr_name = "ovr3";
+	else if (!strcmp("vp4", vp_name))
+		ovr_name = "ovr4";
+	else
+		return NULL;
+
+	return ovr_name;
+}
+
+static void dispc_shared_mode_update_bus_type(struct dispc_features *shared_mode_feat,
+					      struct dispc_device *dispc)
+{
+	u32 i, vp_idx;
+	int num_vps = shared_mode_feat->num_vps;
+	enum dispc_vp_bus_type vp_bus_type[TIDSS_MAX_PORTS];
+
+	memcpy((void *)vp_bus_type, (void *)shared_mode_feat->vp_bus_type,
+	       sizeof(vp_bus_type));
+	memset(shared_mode_feat->vp_bus_type, 0, sizeof(vp_bus_type));
+
+	for (i = 0; i < num_vps; i++) {
+		/*
+		 * Find corresponding vp bus type.
+		 */
+		vp_idx = get_vp_idx_from_vp(shared_mode_feat->vp_name[i]);
+		shared_mode_feat->vp_bus_type[i] = vp_bus_type[vp_idx];
+	}
+}
+
+static int dispc_update_shared_mode_features(struct dispc_features *shared_mode_feat,
+					     struct dispc_device *dispc)
+{
+	int r = 0, i = 0;
+
+	dev_dbg(dispc->dev, "Start updating dispc feature list for shared mode:\n");
+
+	/*
+	 * Start with a shallow copy from existing features and prune the list
+	 * as per what is actually made available to Linux
+	 */
+	memcpy((void *)shared_mode_feat, (void *)dispc->feat, sizeof(*shared_mode_feat));
+	shared_mode_feat->num_vps = device_property_string_array_count(dispc->dev,
+								       "ti,dss-shared-mode-vp");
+	shared_mode_feat->num_planes = device_property_string_array_count(dispc->dev,
+									  "ti,dss-shared-mode-planes");
+
+	r = device_property_read_string(dispc->dev, "ti,dss-shared-mode-common",
+					(const char **)&shared_mode_feat->common);
+	if (r) {
+		dev_err(dispc->dev, "failed to read shared video port name: %d\n", r);
+		return r;
+	}
+
+	memset(shared_mode_feat->vid_name, 0, sizeof(shared_mode_feat->vid_name));
+	r = device_property_read_string_array(dispc->dev, "ti,dss-shared-mode-planes",
+					      shared_mode_feat->vid_name, TIDSS_MAX_PLANES);
+	if (r < 0) {
+		dev_err(dispc->dev, "failed to read client vid layer name: %d\n", r);
+		return r;
+	}
+
+	r = device_property_read_u32_array(dispc->dev, "ti,dss-shared-mode-vp-owned",
+					   dispc->tidss->shared_mode_owned_vps,
+					   shared_mode_feat->num_vps);
+	if (r < 0) {
+		dev_err(dispc->dev, "failed to read owned vp list: %d\n", r);
+		return r;
+	}
+
+	memset(shared_mode_feat->vp_name, 0, sizeof(shared_mode_feat->vp_name));
+	r = device_property_read_string_array(dispc->dev, "ti,dss-shared-mode-vp",
+					      shared_mode_feat->vp_name, TIDSS_MAX_PORTS);
+	if (r < 0) {
+		dev_err(dispc->dev, "failed to read shared video port name: %d\n", r);
+		return r;
+	}
+
+	memset(shared_mode_feat->vid_order, 0, sizeof(shared_mode_feat->vid_order));
+	r = device_property_read_u32_array(dispc->dev, "ti,dss-shared-mode-plane-zorder",
+					   shared_mode_feat->vid_order,
+					   shared_mode_feat->num_planes);
+	if (r < 0) {
+		dev_err(dispc->dev, "failed to read vid_order array name: %d\n", r);
+		return r;
+	}
+	memcpy((void *)shared_mode_feat->vpclk_name, (void *)shared_mode_feat->vp_name,
+	       sizeof(shared_mode_feat->vpclk_name));
+	memset(shared_mode_feat->ovr_name, 0, sizeof(shared_mode_feat->ovr_name));
+
+	for (i = 0; i < shared_mode_feat->num_vps; i++) {
+		shared_mode_feat->ovr_name[i] = get_ovr_from_vp(shared_mode_feat->vp_name[i]);
+		dev_dbg(dispc->dev, "vp[%d] = %s, ovr[%d] = %s vpclk[%d] = %s vp_owned[%d] = %d\n",
+			i,  shared_mode_feat->vp_name[i], i, shared_mode_feat->ovr_name[i], i,
+			shared_mode_feat->vpclk_name[i], i, dispc->tidss->shared_mode_owned_vps[i]);
+	}
+
+	for (i = 0; i < shared_mode_feat->num_planes; i++) {
+		if (!strncmp("vidl", shared_mode_feat->vid_name[i], 4))
+			shared_mode_feat->vid_lite[i] = true;
+		dev_dbg(dispc->dev, "vid[%d] = %s, vid_order[%d] = %u vid_lite[%d] = %u\n", i,
+			shared_mode_feat->vid_name[i], i, shared_mode_feat->vid_order[i], i,
+			shared_mode_feat->vid_lite[i]);
+	}
+
+	if (!strcmp(shared_mode_feat->common, "common1"))
+		shared_mode_feat->common_regs = tidss_am62_common1_regs;
+
+	dev_dbg(dispc->dev, "common : %s\n", shared_mode_feat->common);
+	dispc_shared_mode_update_bus_type(shared_mode_feat, dispc);
+	dev_dbg(dispc->dev, "Feature list updated for shared mode\n");
+
+	return 0;
+}
+
 void dispc_vp_setup(struct dispc_device *dispc, u32 hw_videoport,
 		    const struct drm_crtc_state *state, bool newmodeset)
 {
@@ -2662,6 +2834,16 @@ void dispc_vp_setup(struct dispc_device *dispc, u32 hw_videoport,
 	dispc_vp_set_color_mgmt(dispc, hw_videoport, state, newmodeset);
 }
 
+static bool dispc_owns_global_common_in_shared_mode(struct dispc_device *dispc)
+{
+	if ((!strcmp(dispc->feat->common, "common") ||
+	     !strcmp(dispc->feat->common, "common_m")) &&
+	     dispc->tidss->shared_mode)
+		return true;
+	else
+		return false;
+}
+
 int dispc_runtime_suspend(struct dispc_device *dispc)
 {
 	dev_dbg(dispc->dev, "suspend\n");
@@ -2846,6 +3028,7 @@ int dispc_init(struct tidss_device *tidss)
 	struct platform_device *pdev = to_platform_device(dev);
 	struct dispc_device *dispc;
 	const struct dispc_features *feat;
+	struct dispc_features *shared_mode_feat;
 	unsigned int i, num_fourccs;
 	int r = 0;
 
@@ -2887,6 +3070,21 @@ int dispc_init(struct tidss_device *tidss)
 
 	dispc->num_fourccs = num_fourccs;
 
+	if (tidss->shared_mode) {
+		dev_dbg(dev, "%s : DSS is being shared with remote core\n", __func__);
+		shared_mode_feat = devm_kzalloc(dev, sizeof(*shared_mode_feat), GFP_KERNEL);
+		if (!shared_mode_feat)
+			return -ENOMEM;
+
+		r = dispc_update_shared_mode_features(shared_mode_feat, dispc);
+		if (r)
+			return r;
+
+		tidss->feat = (const struct dispc_features *)shared_mode_feat;
+		feat = tidss->feat;
+		dispc->feat = feat;
+	}
+
 	dispc_common_regmap = dispc->feat->common_regs;
 
 	r = dispc_iomap_resource(pdev, dispc->feat->common,
@@ -2933,25 +3131,37 @@ int dispc_init(struct tidss_device *tidss)
 	}
 
 	if (feat->subrev == DISPC_AM65X) {
-		r = dispc_init_am65x_oldi_io_ctrl(dev, dispc);
-		if (r)
-			return r;
+		/*
+		 * For shared mode, Initialize the OLDI IO control only if we own
+		 * the OLDI Tx ports
+		 */
+		if (!tidss->shared_mode || tidss->shared_mode_own_oldi) {
+			r = dispc_init_am65x_oldi_io_ctrl(dev, dispc);
+			if (r)
+				return r;
+		}
 	}
 
-	dispc->fclk = devm_clk_get(dev, "fck");
-	if (IS_ERR(dispc->fclk)) {
-		dev_err(dev, "%s: Failed to get fclk: %ld\n",
-			__func__, PTR_ERR(dispc->fclk));
-		return PTR_ERR(dispc->fclk);
-	}
-	dev_dbg(dev, "DSS fclk %lu Hz\n", clk_get_rate(dispc->fclk));
+	/*
+	 * For shared mode, Initialize the hardware and clocking only if processing core running
+	 * Linux has ownership of DSS global register space
+	 */
+	if (!tidss->shared_mode || dispc_owns_global_common_in_shared_mode(dispc)) {
+		dispc->fclk = devm_clk_get(dev, "fck");
+		if (IS_ERR(dispc->fclk)) {
+			dev_err(dev, "%s: Failed to get fclk: %ld\n",
+				__func__, PTR_ERR(dispc->fclk));
+			return PTR_ERR(dispc->fclk);
+		}
+		dev_dbg(dev, "DSS fclk %lu Hz\n", clk_get_rate(dispc->fclk));
 
-	of_property_read_u32(dispc->dev->of_node, "max-memory-bandwidth",
-			     &dispc->memory_bandwidth_limit);
+		of_property_read_u32(dispc->dev->of_node, "max-memory-bandwidth",
+				     &dispc->memory_bandwidth_limit);
 
-	r = dispc_init_hw(dispc);
-	if (r)
-		return r;
+		r = dispc_init_hw(dispc);
+		if (r)
+			return r;
+	}
 
 	tidss->dispc = dispc;
 
diff --git a/drivers/gpu/drm/tidss/tidss_dispc.h b/drivers/gpu/drm/tidss/tidss_dispc.h
index 086327d51a90..368a39941b34 100644
--- a/drivers/gpu/drm/tidss/tidss_dispc.h
+++ b/drivers/gpu/drm/tidss/tidss_dispc.h
@@ -80,7 +80,7 @@ struct dispc_features {
 	const char *vp_name[TIDSS_MAX_PORTS]; /* Should match dt reg names */
 	const char *ovr_name[TIDSS_MAX_PORTS]; /* Should match dt reg names */
 	const char *vpclk_name[TIDSS_MAX_PORTS]; /* Should match dt clk names */
-	const enum dispc_vp_bus_type vp_bus_type[TIDSS_MAX_PORTS];
+	enum dispc_vp_bus_type vp_bus_type[TIDSS_MAX_PORTS];
 	struct tidss_vp_feat vp_feat;
 	u32 num_planes;
 	const char *vid_name[TIDSS_MAX_PLANES]; /* Should match dt reg names */
diff --git a/drivers/gpu/drm/tidss/tidss_drv.c b/drivers/gpu/drm/tidss/tidss_drv.c
index d15f836dca95..141481635578 100644
--- a/drivers/gpu/drm/tidss/tidss_drv.c
+++ b/drivers/gpu/drm/tidss/tidss_drv.c
@@ -32,6 +32,10 @@ int tidss_runtime_get(struct tidss_device *tidss)
 
 	dev_dbg(tidss->dev, "%s\n", __func__);
 
+	/* No PM in display sharing mode */
+	if (tidss->shared_mode)
+		return 0;
+
 	r = pm_runtime_resume_and_get(tidss->dev);
 	WARN_ON(r < 0);
 	return r;
@@ -43,6 +47,9 @@ void tidss_runtime_put(struct tidss_device *tidss)
 
 	dev_dbg(tidss->dev, "%s\n", __func__);
 
+	if (tidss->shared_mode)
+		return;
+
 	pm_runtime_mark_last_busy(tidss->dev);
 
 	r = pm_runtime_put_autosuspend(tidss->dev);
@@ -140,21 +147,23 @@ static int tidss_probe(struct platform_device *pdev)
 
 	spin_lock_init(&tidss->wait_lock);
 
+	tidss->shared_mode = device_property_read_bool(dev, "ti,dss-shared-mode");
 	ret = dispc_init(tidss);
 	if (ret) {
 		dev_err(dev, "failed to initialize dispc: %d\n", ret);
 		return ret;
 	}
 
-	pm_runtime_enable(dev);
-
-	pm_runtime_set_autosuspend_delay(dev, 1000);
-	pm_runtime_use_autosuspend(dev);
+	if (!tidss->shared_mode) {
+		pm_runtime_enable(dev);
+		pm_runtime_set_autosuspend_delay(dev, 1000);
+		pm_runtime_use_autosuspend(dev);
 
 #ifndef CONFIG_PM
-	/* If we don't have PM, we need to call resume manually */
-	dispc_runtime_resume(tidss->dispc);
+		/* If we don't have PM, we need to call resume manually */
+		dispc_runtime_resume(tidss->dispc);
 #endif
+	}
 
 	ret = tidss_modeset_init(tidss);
 	if (ret < 0) {
@@ -196,6 +205,8 @@ static int tidss_probe(struct platform_device *pdev)
 	tidss_irq_uninstall(ddev);
 
 err_runtime_suspend:
+	if (tidss->shared_mode)
+		return ret;
 #ifndef CONFIG_PM
 	dispc_runtime_suspend(tidss->dispc);
 #endif
@@ -219,12 +230,14 @@ static void tidss_remove(struct platform_device *pdev)
 
 	tidss_irq_uninstall(ddev);
 
+	if (!tidss->shared_mode) {
 #ifndef CONFIG_PM
-	/* If we don't have PM, we need to call suspend manually */
-	dispc_runtime_suspend(tidss->dispc);
+		/* If we don't have PM, we need to call suspend manually */
+		dispc_runtime_suspend(tidss->dispc);
 #endif
-	pm_runtime_dont_use_autosuspend(dev);
-	pm_runtime_disable(dev);
+		pm_runtime_dont_use_autosuspend(dev);
+		pm_runtime_disable(dev);
+	}
 
 	/* devm allocated dispc goes away with the dev so mark it NULL */
 	dispc_remove(tidss);
diff --git a/drivers/gpu/drm/tidss/tidss_drv.h b/drivers/gpu/drm/tidss/tidss_drv.h
index d7f27b0b0315..68d53c70651d 100644
--- a/drivers/gpu/drm/tidss/tidss_drv.h
+++ b/drivers/gpu/drm/tidss/tidss_drv.h
@@ -31,6 +31,12 @@ struct tidss_device {
 
 	spinlock_t wait_lock;	/* protects the irq masks */
 	dispc_irq_t irq_mask;	/* enabled irqs in addition to wait_list */
+
+	bool shared_mode; /* DSS resources shared between remote core and Linux */
+
+	/* 1: VP owned by Linux 0: VP is owned by remote and shared with Linux */
+	u32 shared_mode_owned_vps[TIDSS_MAX_PORTS];
+	bool shared_mode_own_oldi; /* Linux needs to configure OLDI in shared mode */
 };
 
 #define to_tidss(__dev) container_of(__dev, struct tidss_device, ddev)
-- 
2.34.1

[RFC PATCH 3/3] arm64: dts: ti: k3-am62x: Add overlay to use DSS in display sharing mode

[Devarsh Thakkar]

This overlay needs to be used with display sharing supported device
manager firmware only.

Remote core running this firmware has write access to "common" register
space, VIDL pipeline, OVR1 overlay and VP1 videoport.

The processing core running Linux is provided write access to VID
pipeline and "common1" register space.

The VP1 video port is shared between Linux and remote core with remote
core configuring the overlay manager to set Zorder 1 for VID pipeline
and Zorder 2 for VIDL pipeline.

Add reserved memory region for framebuffer region used by remote core in
dss shared mode overlay file so that Linux does not re-use the same
while allocating memory.

Also add a label for reserved memory region in base device-tree file so
that it can be referred back in overlay file.

Signed-off-by: Devarsh Thakkar <devarsht@ti.com>
---
 arch/arm64/boot/dts/ti/Makefile               |  1 +
 .../arm64/boot/dts/ti/k3-am62x-sk-common.dtsi |  2 +-
 .../dts/ti/k3-am62x-sk-dss-shared-mode.dtso   | 33 +++++++++++++++++++
 3 files changed, 35 insertions(+), 1 deletion(-)
 create mode 100644 arch/arm64/boot/dts/ti/k3-am62x-sk-dss-shared-mode.dtso

diff --git a/arch/arm64/boot/dts/ti/Makefile b/arch/arm64/boot/dts/ti/Makefile
index 52c1dc910308..ff832741b367 100644
--- a/arch/arm64/boot/dts/ti/Makefile
+++ b/arch/arm64/boot/dts/ti/Makefile
@@ -35,6 +35,7 @@ dtb-$(CONFIG_ARCH_K3) += k3-am62x-sk-csi2-ov5640.dtbo
 dtb-$(CONFIG_ARCH_K3) += k3-am62x-sk-csi2-tevi-ov5640.dtbo
 dtb-$(CONFIG_ARCH_K3) += k3-am62x-sk-csi2-imx219.dtbo
 dtb-$(CONFIG_ARCH_K3) += k3-am62x-sk-hdmi-audio.dtbo
+dtb-$(CONFIG_ARCH_K3) += k3-am62x-sk-dss-shared-mode.dtbo
 
 # Boards with AM64x SoC
 dtb-$(CONFIG_ARCH_K3) += k3-am642-evm.dtb
diff --git a/arch/arm64/boot/dts/ti/k3-am62x-sk-common.dtsi b/arch/arm64/boot/dts/ti/k3-am62x-sk-common.dtsi
index 33768c02d8eb..8b55ca10102f 100644
--- a/arch/arm64/boot/dts/ti/k3-am62x-sk-common.dtsi
+++ b/arch/arm64/boot/dts/ti/k3-am62x-sk-common.dtsi
@@ -34,7 +34,7 @@ memory@80000000 {
 		reg = <0x00000000 0x80000000 0x00000000 0x80000000>;
 	};
 
-	reserved-memory {
+	reserved_memory: reserved-memory {
 		#address-cells = <2>;
 		#size-cells = <2>;
 		ranges;
diff --git a/arch/arm64/boot/dts/ti/k3-am62x-sk-dss-shared-mode.dtso b/arch/arm64/boot/dts/ti/k3-am62x-sk-dss-shared-mode.dtso
new file mode 100644
index 000000000000..02153748a5c2
--- /dev/null
+++ b/arch/arm64/boot/dts/ti/k3-am62x-sk-dss-shared-mode.dtso
@@ -0,0 +1,33 @@
+// SPDX-License-Identifier: GPL-2.0
+/**
+ * DT overlay to enable display sharing mode for AM62P DSS0
+ * This is compatible with custom AM62 Device Manager firmware
+ *
+ * Copyright (C) 2023 Texas Instruments Incorporated - http://www.ti.com/
+ */
+
+/dts-v1/;
+/plugin/;
+
+#include <dt-bindings/interrupt-controller/irq.h>
+#include <dt-bindings/interrupt-controller/arm-gic.h>
+
+&dss0 {
+	ti,dss-shared-mode;
+	ti,dss-shared-mode-vp = "vp1";
+	ti,dss-shared-mode-vp-owned = <0>;
+	ti,dss-shared-mode-common = "common1";
+	ti,dss-shared-mode-planes = "vid";
+	ti,dss-shared-mode-plane-zorder = <0>;
+	interrupts = <GIC_SPI 85 IRQ_TYPE_LEVEL_HIGH>;
+};
+
+&reserved_memory {
+	#address-cells = <2>;
+	#size-cells = <2>;
+	rtos_framebuffer_memory_region: rtos-framebuffer-memory@94800000 {
+		compatible = "shared-dma-pool";
+		reg = <0x00 0x94800000 0x00 0x08000000>;
+		no-map;
+	};
+};
-- 
2.34.1

Re: [RFC PATCH 1/3] dt-bindings: display: ti,am65x-dss: Add support for display sharing mode

[Rob Herring]

On Tue, Jan 16, 2024 at 07:11:40PM +0530, Devarsh Thakkar wrote:
> Add support for using TI Keystone DSS hardware present in display
> sharing mode.
> 
> TI Keystone DSS hardware supports partitioning of resources between
> multiple hosts as it provides separate register space and unique
> interrupt line to each host.
> 
> The DSS hardware can be used in shared mode in such a way that one or
> more of video planes can be owned by Linux wherease other planes can be
> owned by remote cores.
> 
> One or more of the video ports can be dedicated exclusively to a
> processing core, wherease some of the video ports can be shared between
> two hosts too with only one of them having write access.
> 
> Signed-off-by: Devarsh Thakkar <devarsht@ti.com>
> ---
>  .../bindings/display/ti/ti,am65x-dss.yaml     | 82 +++++++++++++++++++
>  1 file changed, 82 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml b/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml
> index 55e3e490d0e6..d9bc69fbf1fb 100644
> --- a/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml
> +++ b/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml
> @@ -112,6 +112,86 @@ properties:
>        Input memory (from main memory to dispc) bandwidth limit in
>        bytes per second
>  
> +  ti,dss-shared-mode:
> +    type: boolean
> +    description:
> +      TI DSS7 supports sharing of display between multiple hosts
> +      as it provides separate register space for display configuration and
> +      unique interrupt line to each host.

If you care about line breaks, you need '|'. 

> +      One of the host is provided access to the global display
> +      configuration labelled as "common" region of DSS allows that host
> +      exclusive access to global registers of DSS while other host can
> +      configure the display for it's usage using a separate register
> +      space labelled as "common1".
> +      The DSS resources can be partitioned in such a way that one or more
> +      of the video planes are owned by Linux whereas other video planes

Your h/w can only run Linux?

What if you want to use this same binding to define the configuration to 
the 'remote processor'? You can easily s/Linux/the OS/, but it all 
should be reworded to describe things in terms of the local processor.

> +      can be owned by a remote core.
> +      The video port controlling these planes acts as a shared video port
> +      and it can be configured with write access either by Linux or the
> +      remote core in which case Linux only has read-only access to that
> +      video port.

What is the purpose of this property when all the other properties are 
required?

> +
> +  ti,dss-shared-mode-planes:
> +    description:
> +      The video layer that is owned by processing core running Linux.
> +      The display driver running from Linux has exclusive write access to
> +      this video layer.
> +    $ref: /schemas/types.yaml#/definitions/string
> +    enum: [vidl, vid]
> +
> +  ti,dss-shared-mode-vp:
> +    description:
> +      The video port that is being used in context of processing core
> +      running Linux with display susbsytem being used in shared mode.
> +      This can be owned either by the processing core running Linux in
> +      which case Linux has the write access and the responsibility to
> +      configure this video port and the associated overlay manager or
> +      it can be shared between core running Linux and a remote core
> +      with remote core provided with write access to this video port and
> +      associated overlay managers and remote core configures and drives
> +      this video port also feeding data from one or more of the
> +      video planes owned by Linux, with Linux only having read-only access
> +      to this video port and associated overlay managers.
> +
> +    $ref: /schemas/types.yaml#/definitions/string
> +    enum: [vp1, vp2]
> +
> +  ti,dss-shared-mode-common:
> +    description:
> +      The DSS register region owned by processing core running Linux.
> +    $ref: /schemas/types.yaml#/definitions/string
> +    enum: [common, common1]
> +
> +  ti,dss-shared-mode-vp-owned:
> +    description:
> +      This tells whether processing core running Linux has write access to
> +      the video ports enlisted in ti,dss-shared-mode-vps.
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    enum: [0, 1]

This can be boolean. Do writes abort or just get ignored? The latter can 
be probed and doesn't need a property.

> +
> +  ti,dss-shared-mode-plane-zorder:
> +    description:
> +      The zorder of the planes owned by Linux.
> +      For the scenario where Linux is not having write access to associated
> +      video port, this field is just for
> +      informational purpose to enumerate the zorder configuration
> +      being used by remote core.
> +
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    enum: [0, 1]

I don't understand how 0 or 1 defines Z-order.

> +
> +dependencies:
> +  ti,dss-shared-mode: [ 'ti,dss-shared-mode-planes', 'ti,dss-shared-mode-vp',
> +                        'ti,dss-shared-mode-plane-zorder', 'ti,dss-shared-mode-vp-owned']
> +  ti,dss-shared-mode-vp: ['ti,dss-shared-mode', 'ti,dss-shared-mode-planes',
> +                          'ti,dss-shared-mode-plane-zorder', 'ti,dss-shared-mode-vp-owned']
> +  ti,dss-shared-mode-planes: ['ti,dss-shared-mode', 'ti,dss-shared-mode-vp',
> +                              'ti,dss-shared-mode-plane-zorder', 'ti,dss-shared-mode-vp-owned']
> +  ti,dss-shared-mode-plane-zorder: ['ti,dss-shared-mode-planes', 'ti,dss-shared-mode-vp',
> +                                    'ti,dss-shared-mode', 'ti,dss-shared-mode-vp-owned']
> +  ti,dss-shared-mode-vp-owned: ['ti,dss-shared-mode-planes', 'ti,dss-shared-mode-vp',
> +                                'ti,dss-shared-mode', 'ti,dss-shared-mode-plane-zorder']
> +
>  allOf:
>    - if:
>        properties:
> @@ -123,6 +203,8 @@ allOf:
>          ports:
>            properties:
>              port@0: false
> +            ti,dss-shared-mode-vp:
> +            enum: [vp2]

This should throw a warning. You just defined a property called 'enum'.

Rob

Re: [RFC PATCH 1/3] dt-bindings: display: ti,am65x-dss: Add support for display sharing mode

[Rob Herring]

On Wed, Jan 17, 2024 at 02:13:42PM -0600, Rob Herring wrote:
> On Tue, Jan 16, 2024 at 07:11:40PM +0530, Devarsh Thakkar wrote:
> > Add support for using TI Keystone DSS hardware present in display
> > sharing mode.
> > 
> > TI Keystone DSS hardware supports partitioning of resources between
> > multiple hosts as it provides separate register space and unique
> > interrupt line to each host.
> > 
> > The DSS hardware can be used in shared mode in such a way that one or
> > more of video planes can be owned by Linux wherease other planes can be
> > owned by remote cores.
> > 
> > One or more of the video ports can be dedicated exclusively to a
> > processing core, wherease some of the video ports can be shared between
> > two hosts too with only one of them having write access.
> > 
> > Signed-off-by: Devarsh Thakkar <devarsht@ti.com>
> > ---
> >  .../bindings/display/ti/ti,am65x-dss.yaml     | 82 +++++++++++++++++++
> >  1 file changed, 82 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml b/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml
> > index 55e3e490d0e6..d9bc69fbf1fb 100644
> > --- a/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml
> > +++ b/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml
> > @@ -112,6 +112,86 @@ properties:
> >        Input memory (from main memory to dispc) bandwidth limit in
> >        bytes per second
> >  
> > +  ti,dss-shared-mode:
> > +    type: boolean
> > +    description:
> > +      TI DSS7 supports sharing of display between multiple hosts
> > +      as it provides separate register space for display configuration and
> > +      unique interrupt line to each host.
> 
> If you care about line breaks, you need '|'. 
> 
> > +      One of the host is provided access to the global display
> > +      configuration labelled as "common" region of DSS allows that host
> > +      exclusive access to global registers of DSS while other host can
> > +      configure the display for it's usage using a separate register
> > +      space labelled as "common1".
> > +      The DSS resources can be partitioned in such a way that one or more
> > +      of the video planes are owned by Linux whereas other video planes
> 
> Your h/w can only run Linux?
> 
> What if you want to use this same binding to define the configuration to 
> the 'remote processor'? You can easily s/Linux/the OS/, but it all 
> should be reworded to describe things in terms of the local processor.
> 
> > +      can be owned by a remote core.
> > +      The video port controlling these planes acts as a shared video port
> > +      and it can be configured with write access either by Linux or the
> > +      remote core in which case Linux only has read-only access to that
> > +      video port.
> 
> What is the purpose of this property when all the other properties are 
> required?
> 
> > +
> > +  ti,dss-shared-mode-planes:
> > +    description:
> > +      The video layer that is owned by processing core running Linux.
> > +      The display driver running from Linux has exclusive write access to
> > +      this video layer.
> > +    $ref: /schemas/types.yaml#/definitions/string
> > +    enum: [vidl, vid]
> > +
> > +  ti,dss-shared-mode-vp:
> > +    description:
> > +      The video port that is being used in context of processing core
> > +      running Linux with display susbsytem being used in shared mode.
> > +      This can be owned either by the processing core running Linux in
> > +      which case Linux has the write access and the responsibility to
> > +      configure this video port and the associated overlay manager or
> > +      it can be shared between core running Linux and a remote core
> > +      with remote core provided with write access to this video port and
> > +      associated overlay managers and remote core configures and drives
> > +      this video port also feeding data from one or more of the
> > +      video planes owned by Linux, with Linux only having read-only access
> > +      to this video port and associated overlay managers.
> > +
> > +    $ref: /schemas/types.yaml#/definitions/string
> > +    enum: [vp1, vp2]
> > +
> > +  ti,dss-shared-mode-common:
> > +    description:
> > +      The DSS register region owned by processing core running Linux.
> > +    $ref: /schemas/types.yaml#/definitions/string
> > +    enum: [common, common1]
> > +
> > +  ti,dss-shared-mode-vp-owned:
> > +    description:
> > +      This tells whether processing core running Linux has write access to
> > +      the video ports enlisted in ti,dss-shared-mode-vps.
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    enum: [0, 1]
> 
> This can be boolean. Do writes abort or just get ignored? The latter can 
> be probed and doesn't need a property.
> 
> > +
> > +  ti,dss-shared-mode-plane-zorder:
> > +    description:
> > +      The zorder of the planes owned by Linux.
> > +      For the scenario where Linux is not having write access to associated
> > +      video port, this field is just for
> > +      informational purpose to enumerate the zorder configuration
> > +      being used by remote core.
> > +
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    enum: [0, 1]
> 
> I don't understand how 0 or 1 defines Z-order.
> 
> > +
> > +dependencies:
> > +  ti,dss-shared-mode: [ 'ti,dss-shared-mode-planes', 'ti,dss-shared-mode-vp',
> > +                        'ti,dss-shared-mode-plane-zorder', 'ti,dss-shared-mode-vp-owned']
> > +  ti,dss-shared-mode-vp: ['ti,dss-shared-mode', 'ti,dss-shared-mode-planes',
> > +                          'ti,dss-shared-mode-plane-zorder', 'ti,dss-shared-mode-vp-owned']
> > +  ti,dss-shared-mode-planes: ['ti,dss-shared-mode', 'ti,dss-shared-mode-vp',
> > +                              'ti,dss-shared-mode-plane-zorder', 'ti,dss-shared-mode-vp-owned']
> > +  ti,dss-shared-mode-plane-zorder: ['ti,dss-shared-mode-planes', 'ti,dss-shared-mode-vp',
> > +                                    'ti,dss-shared-mode', 'ti,dss-shared-mode-vp-owned']
> > +  ti,dss-shared-mode-vp-owned: ['ti,dss-shared-mode-planes', 'ti,dss-shared-mode-vp',
> > +                                'ti,dss-shared-mode', 'ti,dss-shared-mode-plane-zorder']
> > +
> >  allOf:
> >    - if:
> >        properties:
> > @@ -123,6 +203,8 @@ allOf:
> >          ports:
> >            properties:
> >              port@0: false
> > +            ti,dss-shared-mode-vp:
> > +            enum: [vp2]
> 
> This should throw a warning. You just defined a property called 'enum'.

Indeed it does. Test your bindings before sending.

../Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml:204:35: [error] empty value in block mapp
ing (empty-values)                                   
  CHKDT   Documentation/devicetree/bindings/processed-schema.json
/home/rob/proj/linux-dt/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml: allOf:0:then:proper
ties:ports:properties:ti,dss-shared-mode-vp: None is not of type 'object', 'boolean'
        from schema $id: http://json-schema.org/draft-07/schema#
/home/rob/proj/linux-dt/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml: allOf:0:then:proper
ties:ports:properties:enum: ['vp2'] is not of type 'object', 'boolean'
        from schema $id: http://json-schema.org/draft-07/schema#
/home/rob/proj/linux-dt/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml: allOf:0:then:proper
ties:ports:properties: 'enum' should not be valid under {'$ref': '#/definitions/json-schema-prop-names'}
        hint: A json-schema keyword was found instead of a DT property name.
        from schema $id: http://devicetree.org/meta-schemas/keywords.yaml# 
/home/rob/proj/linux-dt/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml: allOf:0:then:proper
ties:ports:properties:ti,dss-shared-mode-vp: None is not of type 'object', 'boolean'
        from schema $id: http://devicetree.org/meta-schemas/keywords.yaml#
/home/rob/proj/linux-dt/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml: allOf:0:then:proper
ties:ports:properties:enum: ['vp2'] is not of type 'object', 'boolean'
        from schema $id: http://devicetree.org/meta-schemas/keywords.yaml#

Re: [RFC PATCH 1/3] dt-bindings: display: ti,am65x-dss: Add support for display sharing mode

[Devarsh Thakkar]

Hi Rob,

Thanks for the quick review.

On 18/01/24 01:43, Rob Herring wrote:
> On Tue, Jan 16, 2024 at 07:11:40PM +0530, Devarsh Thakkar wrote:
>> Add support for using TI Keystone DSS hardware present in display
>> sharing mode.
>>
>> TI Keystone DSS hardware supports partitioning of resources between
>> multiple hosts as it provides separate register space and unique
>> interrupt line to each host.
>>
>> The DSS hardware can be used in shared mode in such a way that one or
>> more of video planes can be owned by Linux wherease other planes can be
>> owned by remote cores.
>>
>> One or more of the video ports can be dedicated exclusively to a
>> processing core, wherease some of the video ports can be shared between
>> two hosts too with only one of them having write access.
>>
>> Signed-off-by: Devarsh Thakkar <devarsht@ti.com>
>> ---
>>  .../bindings/display/ti/ti,am65x-dss.yaml     | 82 +++++++++++++++++++
>>  1 file changed, 82 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml b/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml
>> index 55e3e490d0e6..d9bc69fbf1fb 100644
>> --- a/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml
>> +++ b/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml
>> @@ -112,6 +112,86 @@ properties:
>>        Input memory (from main memory to dispc) bandwidth limit in
>>        bytes per second
>>  
>> +  ti,dss-shared-mode:
>> +    type: boolean
>> +    description:
>> +      TI DSS7 supports sharing of display between multiple hosts
>> +      as it provides separate register space for display configuration and
>> +      unique interrupt line to each host.
> 
> If you care about line breaks, you need '|'. 
> 

Noted.

>> +      One of the host is provided access to the global display
>> +      configuration labelled as "common" region of DSS allows that host
>> +      exclusive access to global registers of DSS while other host can
>> +      configure the display for it's usage using a separate register
>> +      space labelled as "common1".
>> +      The DSS resources can be partitioned in such a way that one or more
>> +      of the video planes are owned by Linux whereas other video planes
> 
> Your h/w can only run Linux?
> 
> What if you want to use this same binding to define the configuration to 
> the 'remote processor'? You can easily s/Linux/the OS/, but it all 
> should be reworded to describe things in terms of the local processor.
> 

It can run both Linux and RTOS or for that matter any other OS too. But yes I
got your point, will reword accordingly.

>> +      can be owned by a remote core.
>> +      The video port controlling these planes acts as a shared video port
>> +      and it can be configured with write access either by Linux or the
>> +      remote core in which case Linux only has read-only access to that
>> +      video port.
> 
> What is the purpose of this property when all the other properties are 
> required?
> 

The ti,dss-shared-mode and below group of properties are optional. But
if ti,dss-shared-mode is set then only driver should parse below set of
properties.

>> +
>> +  ti,dss-shared-mode-planes:
>> +    description:
>> +      The video layer that is owned by processing core running Linux.
>> +      The display driver running from Linux has exclusive write access to
>> +      this video layer.
>> +    $ref: /schemas/types.yaml#/definitions/string
>> +    enum: [vidl, vid]
>> +
>> +  ti,dss-shared-mode-vp:
>> +    description:
>> +      The video port that is being used in context of processing core
>> +      running Linux with display susbsytem being used in shared mode.
>> +      This can be owned either by the processing core running Linux in
>> +      which case Linux has the write access and the responsibility to
>> +      configure this video port and the associated overlay manager or
>> +      it can be shared between core running Linux and a remote core
>> +      with remote core provided with write access to this video port and
>> +      associated overlay managers and remote core configures and drives
>> +      this video port also feeding data from one or more of the
>> +      video planes owned by Linux, with Linux only having read-only access
>> +      to this video port and associated overlay managers.
>> +
>> +    $ref: /schemas/types.yaml#/definitions/string
>> +    enum: [vp1, vp2]
>> +
>> +  ti,dss-shared-mode-common:
>> +    description:
>> +      The DSS register region owned by processing core running Linux.
>> +    $ref: /schemas/types.yaml#/definitions/string
>> +    enum: [common, common1]
>> +
>> +  ti,dss-shared-mode-vp-owned:
>> +    description:
>> +      This tells whether processing core running Linux has write access to
>> +      the video ports enlisted in ti,dss-shared-mode-vps.
>> +    $ref: /schemas/types.yaml#/definitions/uint32
>> +    enum: [0, 1]
> 
> This can be boolean. Do writes abort or just get ignored? The latter can 
> be probed and doesn't need a property.
> 

Although we have kept all these properties as enums, but actually in driver we
are treating them as array of enums and using device_property_read_u32_array.

The reason being that for SoCs using am65x-dss bindings they can only have
single entry either vp1 or vp2 or 0 or 1 as there are only two video ports. So
for them the device tree overlay would look like :
&dss0 {

        ti,dss-shared-mode;

        ti,dss-shared-mode-vp = "vp1";

        ti,dss-shared-mode-vp-owned = <0>;

        ti,dss-shared-mode-common = "common1";

        ti,dss-shared-mode-planes = "vid";

        ti,dss-shared-mode-plane-zorder = <0>;

        interrupts = <GIC_SPI 85 IRQ_TYPE_LEVEL_>;
}

But we also plan to extend these bindings to SoCs using
Documentation/devicetree/bindings/display/ti/ti,j721e-dss.yaml where there are
multiple video ports. So in that the driver and bindings should support below
configuration :

&dss0 {

        ti,dss-shared-mode;

        ti,dss-shared-mode-vp = "vp1 vp2";

        ti,dss-shared-mode-vp-owned = <0 1>;

        ti,dss-shared-mode-common = "common_s1";

        ti,dss-shared-mode-planes = "vid1 vidl1";

        ti,dss-shared-mode-plane-zorder = <0 1>;

        interrupts = <GIC_SPI 85 IRQ_TYPE_LEVEL_>;
}

As I am using device_property_read_u32_array in driver I thought to keep this
as uint32 in enum for am65x.yaml which works well with the driver.

>> +
>> +  ti,dss-shared-mode-plane-zorder:
>> +    description:
>> +      The zorder of the planes owned by Linux.
>> +      For the scenario where Linux is not having write access to associated
>> +      video port, this field is just for
>> +      informational purpose to enumerate the zorder configuration
>> +      being used by remote core.
>> +
>> +    $ref: /schemas/types.yaml#/definitions/uint32
>> +    enum: [0, 1]
> 
> I don't understand how 0 or 1 defines Z-order.
> 

As there are only two planes in total so z-order can be either 0 or 1 for the
shared mode plane as there is only a single entry of plane.
For e.g. if ti,dss-shared-mode-plane-zorder is 1 then it means the plane owned
by Linux is programmed as topmost plane wherease the plane owned by remote
core is programmed as the underneath one.

>> +
>> +dependencies:
>> +  ti,dss-shared-mode: [ 'ti,dss-shared-mode-planes', 'ti,dss-shared-mode-vp',
>> +                        'ti,dss-shared-mode-plane-zorder', 'ti,dss-shared-mode-vp-owned']
>> +  ti,dss-shared-mode-vp: ['ti,dss-shared-mode', 'ti,dss-shared-mode-planes',
>> +                          'ti,dss-shared-mode-plane-zorder', 'ti,dss-shared-mode-vp-owned']
>> +  ti,dss-shared-mode-planes: ['ti,dss-shared-mode', 'ti,dss-shared-mode-vp',
>> +                              'ti,dss-shared-mode-plane-zorder', 'ti,dss-shared-mode-vp-owned']
>> +  ti,dss-shared-mode-plane-zorder: ['ti,dss-shared-mode-planes', 'ti,dss-shared-mode-vp',
>> +                                    'ti,dss-shared-mode', 'ti,dss-shared-mode-vp-owned']
>> +  ti,dss-shared-mode-vp-owned: ['ti,dss-shared-mode-planes', 'ti,dss-shared-mode-vp',
>> +                                'ti,dss-shared-mode', 'ti,dss-shared-mode-plane-zorder']
>> +
>>  allOf:
>>    - if:
>>        properties:
>> @@ -123,6 +203,8 @@ allOf:
>>          ports:
>>            properties:
>>              port@0: false
>> +            ti,dss-shared-mode-vp:
>> +            enum: [vp2]
> 
> This should throw a warning. You just defined a property called 'enum'.
> 

Oops will fix this.

Regards
Devarsh

> Rob

Re: [RFC PATCH 1/3] dt-bindings: display: ti,am65x-dss: Add support for display sharing mode

[Rob Herring]

On Thu, Jan 18, 2024 at 7:59 AM Devarsh Thakkar <devarsht@ti.com> wrote:
>
> Hi Rob,
>
> Thanks for the quick review.
>
> On 18/01/24 01:43, Rob Herring wrote:
> > On Tue, Jan 16, 2024 at 07:11:40PM +0530, Devarsh Thakkar wrote:
> >> Add support for using TI Keystone DSS hardware present in display
> >> sharing mode.
> >>
> >> TI Keystone DSS hardware supports partitioning of resources between
> >> multiple hosts as it provides separate register space and unique
> >> interrupt line to each host.
> >>
> >> The DSS hardware can be used in shared mode in such a way that one or
> >> more of video planes can be owned by Linux wherease other planes can be
> >> owned by remote cores.
> >>
> >> One or more of the video ports can be dedicated exclusively to a
> >> processing core, wherease some of the video ports can be shared between
> >> two hosts too with only one of them having write access.
> >>
> >> Signed-off-by: Devarsh Thakkar <devarsht@ti.com>
> >> ---
> >>  .../bindings/display/ti/ti,am65x-dss.yaml     | 82 +++++++++++++++++++
> >>  1 file changed, 82 insertions(+)
> >>
> >> diff --git a/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml b/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml
> >> index 55e3e490d0e6..d9bc69fbf1fb 100644
> >> --- a/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml
> >> +++ b/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml
> >> @@ -112,6 +112,86 @@ properties:
> >>        Input memory (from main memory to dispc) bandwidth limit in
> >>        bytes per second
> >>
> >> +  ti,dss-shared-mode:
> >> +    type: boolean
> >> +    description:
> >> +      TI DSS7 supports sharing of display between multiple hosts
> >> +      as it provides separate register space for display configuration and
> >> +      unique interrupt line to each host.
> >
> > If you care about line breaks, you need '|'.
> >
>
> Noted.
>
> >> +      One of the host is provided access to the global display
> >> +      configuration labelled as "common" region of DSS allows that host
> >> +      exclusive access to global registers of DSS while other host can
> >> +      configure the display for it's usage using a separate register
> >> +      space labelled as "common1".
> >> +      The DSS resources can be partitioned in such a way that one or more
> >> +      of the video planes are owned by Linux whereas other video planes
> >
> > Your h/w can only run Linux?
> >
> > What if you want to use this same binding to define the configuration to
> > the 'remote processor'? You can easily s/Linux/the OS/, but it all
> > should be reworded to describe things in terms of the local processor.
> >
>
> It can run both Linux and RTOS or for that matter any other OS too. But yes I
> got your point, will reword accordingly.
>
> >> +      can be owned by a remote core.
> >> +      The video port controlling these planes acts as a shared video port
> >> +      and it can be configured with write access either by Linux or the
> >> +      remote core in which case Linux only has read-only access to that
> >> +      video port.
> >
> > What is the purpose of this property when all the other properties are
> > required?
> >
>
> The ti,dss-shared-mode and below group of properties are optional. But
> if ti,dss-shared-mode is set then only driver should parse below set of
> properties.

Let me re-phrase. Drop this property. It serves no purpose since all
the properties have to be present anyway.

> >> +  ti,dss-shared-mode-planes:
> >> +    description:
> >> +      The video layer that is owned by processing core running Linux.
> >> +      The display driver running from Linux has exclusive write access to
> >> +      this video layer.
> >> +    $ref: /schemas/types.yaml#/definitions/string
> >> +    enum: [vidl, vid]
> >> +
> >> +  ti,dss-shared-mode-vp:
> >> +    description:
> >> +      The video port that is being used in context of processing core
> >> +      running Linux with display susbsytem being used in shared mode.
> >> +      This can be owned either by the processing core running Linux in
> >> +      which case Linux has the write access and the responsibility to
> >> +      configure this video port and the associated overlay manager or
> >> +      it can be shared between core running Linux and a remote core
> >> +      with remote core provided with write access to this video port and
> >> +      associated overlay managers and remote core configures and drives
> >> +      this video port also feeding data from one or more of the
> >> +      video planes owned by Linux, with Linux only having read-only access
> >> +      to this video port and associated overlay managers.
> >> +
> >> +    $ref: /schemas/types.yaml#/definitions/string
> >> +    enum: [vp1, vp2]
> >> +
> >> +  ti,dss-shared-mode-common:
> >> +    description:
> >> +      The DSS register region owned by processing core running Linux.
> >> +    $ref: /schemas/types.yaml#/definitions/string
> >> +    enum: [common, common1]
> >> +
> >> +  ti,dss-shared-mode-vp-owned:
> >> +    description:
> >> +      This tells whether processing core running Linux has write access to
> >> +      the video ports enlisted in ti,dss-shared-mode-vps.
> >> +    $ref: /schemas/types.yaml#/definitions/uint32
> >> +    enum: [0, 1]
> >
> > This can be boolean. Do writes abort or just get ignored? The latter can
> > be probed and doesn't need a property.
> >
>
> Although we have kept all these properties as enums, but actually in driver we
> are treating them as array of enums and using device_property_read_u32_array.
>
> The reason being that for SoCs using am65x-dss bindings they can only have
> single entry either vp1 or vp2 or 0 or 1 as there are only two video ports. So
> for them the device tree overlay would look like :
> &dss0 {
>
>         ti,dss-shared-mode;
>
>         ti,dss-shared-mode-vp = "vp1";
>
>         ti,dss-shared-mode-vp-owned = <0>;
>
>         ti,dss-shared-mode-common = "common1";
>
>         ti,dss-shared-mode-planes = "vid";
>
>         ti,dss-shared-mode-plane-zorder = <0>;
>
>         interrupts = <GIC_SPI 85 IRQ_TYPE_LEVEL_>;
> }
>
> But we also plan to extend these bindings to SoCs using
> Documentation/devicetree/bindings/display/ti/ti,j721e-dss.yaml where there are
> multiple video ports. So in that the driver and bindings should support below
> configuration :

What are you waiting for? In that case, all these properties need to
be in a shared schema file and referenced here. Otherwise you will be
defining their types twice (and different types too if some are
changed to arrays).

> &dss0 {
>
>         ti,dss-shared-mode;
>
>         ti,dss-shared-mode-vp = "vp1 vp2";
>
>         ti,dss-shared-mode-vp-owned = <0 1>;
>
>         ti,dss-shared-mode-common = "common_s1";
>
>         ti,dss-shared-mode-planes = "vid1 vidl1";
>
>         ti,dss-shared-mode-plane-zorder = <0 1>;
>
>         interrupts = <GIC_SPI 85 IRQ_TYPE_LEVEL_>;
> }
>
> As I am using device_property_read_u32_array in driver I thought to keep this
> as uint32 in enum for am65x.yaml which works well with the driver.

The type and what accessor functions the kernel uses should match. I
plan to add (debug) checking on this. Debug only because as there's no
type info in the DTB, it all has to be extracted from schemas and put
into the kernel.

> >> +
> >> +  ti,dss-shared-mode-plane-zorder:
> >> +    description:
> >> +      The zorder of the planes owned by Linux.
> >> +      For the scenario where Linux is not having write access to associated
> >> +      video port, this field is just for
> >> +      informational purpose to enumerate the zorder configuration
> >> +      being used by remote core.
> >> +
> >> +    $ref: /schemas/types.yaml#/definitions/uint32
> >> +    enum: [0, 1]
> >
> > I don't understand how 0 or 1 defines Z-order.
> >
>
> As there are only two planes in total so z-order can be either 0 or 1 for the
> shared mode plane as there is only a single entry of plane.
> For e.g. if ti,dss-shared-mode-plane-zorder is 1 then it means the plane owned
> by Linux is programmed as topmost plane wherease the plane owned by remote
> core is programmed as the underneath one.

Please reword the description to make all this clear. The index is the
plane number and value is the z-order with 0 being bottom and N being
the top. I guess this should be an array as well.

Rob

Re: [RFC PATCH 2/3] drm/tidss: Add support for display sharing

[Tomi Valkeinen]

Hi,

On 16/01/2024 15:41, Devarsh Thakkar wrote:
> Display subsystem present in TI Keystone family of devices supports sharing
> of display between multiple hosts as it provides separate register space
> (common* region) for each host to programming display controller and also a
> unique interrupt line for each host.
> 
> This adds support for display sharing, by allowing partitioning of
> resources either at video port level or at video plane level as
> described below :
> 
> 1) Linux can own (i.e have write access) completely one or more of video
> ports along with corresponding resources (viz. overlay managers,
> video planes) used by Linux in context of those video ports.
> Even if Linux is owning
> these video ports it can still share this video port with a remote core
> which can own one or more video planes associated with this video port.
> 
> 2) Linux owns one or more of the video planes with video port
> (along with corresponding overlay manager) associated with these planes
> being owned and controlled by a remote core. Linux still has read-only
> access to the associated video port and overlay managers so that it can
> parse the settings made by remote core.
> 
> For both the cases, the resources used in context of processing core
> running Linux along with ownership information are exposed by user as
> part of device-tree blob and driver uses an updated feature list tailored
> for this shared mode accordingly. The driver also auto-populates
> matching overlay managers and output types from shared video
> port list provided in device-tree blob.
> In dispc_feature struct remove const access specfier for output_type
> array as it is required to be updated dynamically in run-time for shared
> mode.
> 
> For 2) where Linux is only owning a set of video planes with
> corresponding video port and overlay manager controlled by a remote
> core, separate set of CRTC callbacks are used which just latch on
> to the preset mode set by remote core, thus avoiding any reconfiguration
> of associated video ports, overlay managers and clocks.
> For this case, it is also checked that Linux controlled video planes
> don't exceed screen size set by remote core while running the display.
> Display clocks and OLDI related fields are also not
> populated for this scenario as remote core is owning those resources.
> 
> For 1), where Linux owns only a set of video port and associated
> planes with rest of resources owned completely by remote cores,
> only those set of resources are exposed to Linux and programmed using
> traditional CRTC helpers and rest of video ports and associated resources
> are removed from feature list accordingly.
> 
> Signed-off-by: Devarsh Thakkar <devarsht@ti.com>
> ---
>   drivers/gpu/drm/tidss/tidss_crtc.c  | 120 ++++++++++++-
>   drivers/gpu/drm/tidss/tidss_dispc.c | 254 +++++++++++++++++++++++++---
>   drivers/gpu/drm/tidss/tidss_dispc.h |   2 +-
>   drivers/gpu/drm/tidss/tidss_drv.c   |  33 ++--
>   drivers/gpu/drm/tidss/tidss_drv.h   |   6 +
>   5 files changed, 375 insertions(+), 40 deletions(-)
> 
> diff --git a/drivers/gpu/drm/tidss/tidss_crtc.c b/drivers/gpu/drm/tidss/tidss_crtc.c
> index 5f838980c7a1..f6a877ff4c6c 100644
> --- a/drivers/gpu/drm/tidss/tidss_crtc.c
> +++ b/drivers/gpu/drm/tidss/tidss_crtc.c
> @@ -31,13 +31,19 @@ static void tidss_crtc_finish_page_flip(struct tidss_crtc *tcrtc)
>   	/*
>   	 * New settings are taken into use at VFP, and GO bit is cleared at
>   	 * the same time. This happens before the vertical blank interrupt.
> -	 * So there is a small change that the driver sets GO bit after VFP, but
> +	 * So there is a small chance that the driver sets GO bit after VFP, but
>   	 * before vblank, and we have to check for that case here.
> +	 *
> +	 * For a video port shared between Linux and remote core but owned by remote core,
> +	 * this is not required since Linux just attaches to mode that was preset by remote
> +	 * core with which display is being shared.
>   	 */
> -	busy = dispc_vp_go_busy(tidss->dispc, tcrtc->hw_videoport);
> -	if (busy) {
> -		spin_unlock_irqrestore(&ddev->event_lock, flags);
> -		return;
> +	if (!tidss->shared_mode || tidss->shared_mode_owned_vps[tcrtc->hw_videoport]) {

You test this in multiple places. I think it would be better to combine 
those, in one way or another. Either a helper function, or maybe invert 
the shared_mode_owned_vps, i.e. rather have something like "foreign_vps" 
(better name needed), so that when !shared_mode, the default of False in 
foreign_vps array will just work. Then the above test will be just if 
(!tidss->foreign_vps[tcrtc->hw_videoport])

> +		busy = dispc_vp_go_busy(tidss->dispc, tcrtc->hw_videoport);
> +		if (busy) {
> +			spin_unlock_irqrestore(&ddev->event_lock, flags);
> +			return;
> +		}
>   	}
>   
>   	event = tcrtc->event;
> @@ -208,6 +214,44 @@ static void tidss_crtc_atomic_flush(struct drm_crtc *crtc,
>   	spin_unlock_irqrestore(&ddev->event_lock, flags);
>   }
>   
> +static void tidss_shared_vp_crtc_atomic_flush(struct drm_crtc *crtc,
> +					      struct drm_atomic_state *state)
> +{
> +	struct tidss_crtc *tcrtc = to_tidss_crtc(crtc);
> +	struct drm_device *ddev = crtc->dev;
> +	unsigned long flags;
> +
> +	dev_dbg(ddev->dev,
> +		"%s: %s enabled %d, needs modeset %d, event %p\n", __func__,
> +		crtc->name, drm_atomic_crtc_needs_modeset(crtc->state),
> +		crtc->state->enable, crtc->state->event);
> +
> +	/* There is nothing to do if CRTC is not going to be enabled. */
> +	if (!crtc->state->enable)
> +		return;
> +	/*
> +	 * Flush CRTC changes with go bit only if new modeset is not
> +	 * coming, so CRTC is enabled trough out the commit.
> +	 */
> +	if (drm_atomic_crtc_needs_modeset(crtc->state))
> +		return;
> +
> +	/* We should have event if CRTC is enabled through out this commit. */
> +	if (WARN_ON(!crtc->state->event))
> +		return;
> +
> +	WARN_ON(drm_crtc_vblank_get(crtc) != 0);
> +
> +	spin_lock_irqsave(&ddev->event_lock, flags);
> +
> +	WARN_ON(tcrtc->event);
> +
> +	tcrtc->event = crtc->state->event;
> +	crtc->state->event = NULL;
> +
> +	spin_unlock_irqrestore(&ddev->event_lock, flags);
> +}
> +
>   static void tidss_crtc_atomic_enable(struct drm_crtc *crtc,
>   				     struct drm_atomic_state *state)
>   {
> @@ -253,6 +297,27 @@ static void tidss_crtc_atomic_enable(struct drm_crtc *crtc,
>   	spin_unlock_irqrestore(&ddev->event_lock, flags);
>   }
>   
> +static void tidss_shared_vp_crtc_atomic_enable(struct drm_crtc *crtc,
> +					       struct drm_atomic_state *state)
> +{
> +	struct drm_device *ddev = crtc->dev;
> +	unsigned long flags;
> +
> +	dev_dbg(ddev->dev, "%s, event %p\n", __func__, crtc->state->event);
> +
> +	/* Turn vertical blanking interrupt reporting on. */
> +	drm_crtc_vblank_on(crtc);
> +
> +	spin_lock_irqsave(&ddev->event_lock, flags);
> +
> +	if (crtc->state->event) {
> +		drm_crtc_send_vblank_event(crtc, crtc->state->event);
> +		crtc->state->event = NULL;
> +	}
> +
> +	spin_unlock_irqrestore(&ddev->event_lock, flags);
> +}
> +
>   static void tidss_crtc_atomic_disable(struct drm_crtc *crtc,
>   				      struct drm_atomic_state *state)
>   {
> @@ -288,6 +353,27 @@ static void tidss_crtc_atomic_disable(struct drm_crtc *crtc,
>   	tidss_runtime_put(tidss);
>   }
>   
> +static void tidss_shared_vp_crtc_atomic_disable(struct drm_crtc *crtc,
> +						struct drm_atomic_state *state)
> +{
> +	struct tidss_crtc *tcrtc = to_tidss_crtc(crtc);
> +	struct drm_device *ddev = crtc->dev;
> +	unsigned long flags;
> +
> +	dev_dbg(ddev->dev, "%s, event %p\n", __func__, crtc->state->event);
> +
> +	reinit_completion(&tcrtc->framedone_completion);
> +
> +	spin_lock_irqsave(&ddev->event_lock, flags);
> +	if (crtc->state->event) {
> +		drm_crtc_send_vblank_event(crtc, crtc->state->event);
> +		crtc->state->event = NULL;
> +	}
> +	spin_unlock_irqrestore(&ddev->event_lock, flags);
> +
> +	drm_crtc_vblank_off(crtc);
> +}
> +
>   static
>   enum drm_mode_status tidss_crtc_mode_valid(struct drm_crtc *crtc,
>   					   const struct drm_display_mode *mode)
> @@ -304,7 +390,14 @@ static const struct drm_crtc_helper_funcs tidss_crtc_helper_funcs = {
>   	.atomic_flush = tidss_crtc_atomic_flush,
>   	.atomic_enable = tidss_crtc_atomic_enable,
>   	.atomic_disable = tidss_crtc_atomic_disable,
> +	.mode_valid = tidss_crtc_mode_valid,
> +};
>   
> +static const struct drm_crtc_helper_funcs tidss_shared_vp_crtc_helper_funcs = {
> +	.atomic_check = tidss_crtc_atomic_check,
> +	.atomic_flush = tidss_shared_vp_crtc_atomic_flush,
> +	.atomic_enable = tidss_shared_vp_crtc_atomic_enable,
> +	.atomic_disable = tidss_shared_vp_crtc_atomic_disable,
>   	.mode_valid = tidss_crtc_mode_valid,
>   };
>   
> @@ -406,6 +499,10 @@ struct tidss_crtc *tidss_crtc_create(struct tidss_device *tidss,
>   	bool has_ctm = tidss->feat->vp_feat.color.has_ctm;
>   	int ret;
>   
> +	dev_dbg(tidss->dev, "%s: tidss->shared_mode: %d tidss->shared_mode_owned_vps[%d] = %d\n",
> +		__func__, tidss->shared_mode, hw_videoport,
> +		tidss->shared_mode_owned_vps[hw_videoport]);
> +
>   	tcrtc = kzalloc(sizeof(*tcrtc), GFP_KERNEL);
>   	if (!tcrtc)
>   		return ERR_PTR(-ENOMEM);
> @@ -422,8 +519,17 @@ struct tidss_crtc *tidss_crtc_create(struct tidss_device *tidss,
>   		return ERR_PTR(ret);
>   	}
>   
> -	drm_crtc_helper_add(crtc, &tidss_crtc_helper_funcs);
> -
> +	/* Use shared video port CRTC helpers which don't access associated
> +	 * video port and overlay register spaces when Linux is not owning the
> +	 * video port.
> +	 */
> +	if (tidss->shared_mode && !tidss->shared_mode_owned_vps[hw_videoport]) {
> +		drm_crtc_helper_add(crtc, &tidss_shared_vp_crtc_helper_funcs);
> +		dev_dbg(tidss->dev, "%s: vp%d is being shared with Linux\n", __func__,
> +			hw_videoport + 1);
> +	} else {
> +		drm_crtc_helper_add(crtc, &tidss_crtc_helper_funcs);
> +	}
>   	/*
>   	 * The dispc gamma functions adapt to what ever size we ask
>   	 * from it no matter what HW supports. X-server assumes 256
> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c
> index 1ad711f8d2a8..35a82198629f 100644
> --- a/drivers/gpu/drm/tidss/tidss_dispc.c
> +++ b/drivers/gpu/drm/tidss/tidss_dispc.c
> @@ -273,6 +273,18 @@ const struct dispc_features dispc_j721e_feats = {
>   	.vid_order = { 1, 3, 0, 2 },
>   };
>   
> +static const u16 tidss_am62_common1_regs[DISPC_COMMON_REG_TABLE_LEN] = {
> +	[DISPC_IRQ_EOI_OFF] =			0x24,
> +	[DISPC_IRQSTATUS_RAW_OFF] =		0x28,
> +	[DISPC_IRQSTATUS_OFF] =			0x2c,
> +	[DISPC_IRQENABLE_SET_OFF] =		0x30,
> +	[DISPC_IRQENABLE_CLR_OFF] =		0x40,
> +	[DISPC_VID_IRQENABLE_OFF] =		0x44,
> +	[DISPC_VID_IRQSTATUS_OFF] =		0x58,
> +	[DISPC_VP_IRQENABLE_OFF] =		0x70,
> +	[DISPC_VP_IRQSTATUS_OFF] =		0x7c,
> +};
> +
>   const struct dispc_features dispc_am625_feats = {
>   	.max_pclk_khz = {
>   		[DISPC_VP_DPI] = 165000,
> @@ -1228,6 +1240,22 @@ enum drm_mode_status dispc_vp_mode_valid(struct dispc_device *dispc,
>   
>   	max_pclk = dispc->feat->max_pclk_khz[bus_type];
>   
> +	/*
> +	 * For shared mode, with remote core driving the video port, make sure that Linux
> +	 * controlled primary plane doesn't exceed video port screen size set by remote core
> +	 */
> +	if (dispc->tidss->shared_mode && !dispc->tidss->shared_mode_owned_vps[hw_videoport]) {
> +		int vp_hdisplay = VP_REG_GET(dispc, hw_videoport, DISPC_VP_SIZE_SCREEN, 11, 0) + 1;
> +		int vp_vdisplay = VP_REG_GET(dispc, hw_videoport, DISPC_VP_SIZE_SCREEN, 27, 16) + 1;
> +
> +		if (mode->hdisplay > vp_hdisplay ||
> +		    mode->vdisplay > vp_vdisplay) {
> +			dev_err(dispc->dev, "%dx%d exceeds VP screen size %dx%d in shared mode\n",
> +				mode->hdisplay, mode->vdisplay, vp_hdisplay, vp_vdisplay);
> +			return MODE_BAD;
> +		}
> +	}
> +

I'm not sure I understand this. If the VP is owned by someone else, 
isn't there just a single mode that can be allowed (the one that that 
"someone else" has set up?). The comment also talks about primary 
planes, but this code is about the video mode, not planes.

So my understanding of a VP that's controlled by RTOS is that at boot 
time RTOS figures out a video mode (either the native mode of an LCD, or 
picks a mode from a monitor's EDID), and then that one video mode will 
be used, and never changed.

Linux's tidss can, at probe time probably, look at the VP configuration 
and figure out the video mode details, which can then be exposed to the 
userspace.

Changing video modes at runtime would probably require some kind of IPC.

>   	if (WARN_ON(max_pclk == 0))
>   		return MODE_BAD;
>   
> @@ -1276,15 +1304,18 @@ enum drm_mode_status dispc_vp_mode_valid(struct dispc_device *dispc,
>   		return MODE_BAD_VVALUE;
>   
>   	if (dispc->memory_bandwidth_limit) {
> -		const unsigned int bpp = 4;
> -		u64 bandwidth;
> +		if (!dispc->tidss->shared_mode ||
> +		    dispc->tidss->shared_mode_owned_vps[hw_videoport]) {
> +			const unsigned int bpp = 4;
> +			u64 bandwidth;
>   
> -		bandwidth = 1000 * mode->clock;
> -		bandwidth = bandwidth * mode->hdisplay * mode->vdisplay * bpp;
> -		bandwidth = div_u64(bandwidth, mode->htotal * mode->vtotal);
> +			bandwidth = 1000 * mode->clock;
> +			bandwidth = bandwidth * mode->hdisplay * mode->vdisplay * bpp;
> +			bandwidth = div_u64(bandwidth, mode->htotal * mode->vtotal);
>   
> -		if (dispc->memory_bandwidth_limit < bandwidth)
> -			return MODE_BAD;
> +			if (dispc->memory_bandwidth_limit < bandwidth)
> +				return MODE_BAD;
> +		}
>   	}
>   
>   	return MODE_OK;
> @@ -2655,6 +2686,147 @@ static void dispc_vp_set_color_mgmt(struct dispc_device *dispc,
>   		dispc_k3_vp_set_ctm(dispc, hw_videoport, ctm);
>   }
>   
> +static int get_vp_idx_from_vp(const char *vp_name)
> +{
> +	u32 vp_idx;
> +
> +	if (!strcmp("vp1", vp_name))
> +		vp_idx = 0;
> +	else if (!strcmp("vp2", vp_name))
> +		vp_idx = 1;
> +	else if (!strcmp("vp3", vp_name))
> +		vp_idx = 2;
> +	else if (!strcmp("vp4", vp_name))
> +		vp_idx = 3;
> +	else
> +		return 0;
> +
> +	return vp_idx;
> +}

Any particular reason to use strings in the DT for all there? Using a 
index number in the DT would simplify the driver. We also have lists of 
vp names already in the driver.

I didn't go through the whole logic yet, but these two functions give me 
the feeling that this part could be better.

> +
> +static const char *get_ovr_from_vp(const char *vp_name)
> +{
> +	const char *ovr_name = NULL;
> +
> +	if (!strcmp("vp1", vp_name))
> +		ovr_name = "ovr1";
> +	else if (!strcmp("vp2", vp_name))
> +		ovr_name = "ovr2";
> +	else if (!strcmp("vp3", vp_name))
> +		ovr_name = "ovr3";
> +	else if (!strcmp("vp4", vp_name))
> +		ovr_name = "ovr4";
> +	else
> +		return NULL;
> +
> +	return ovr_name;
> +}
> +
> +static void dispc_shared_mode_update_bus_type(struct dispc_features *shared_mode_feat,
> +					      struct dispc_device *dispc)
> +{
 >
> +	u32 i, vp_idx;
> +	int num_vps = shared_mode_feat->num_vps;
> +	enum dispc_vp_bus_type vp_bus_type[TIDSS_MAX_PORTS];
> +
> +	memcpy((void *)vp_bus_type, (void *)shared_mode_feat->vp_bus_type,
> +	       sizeof(vp_bus_type));
> +	memset(shared_mode_feat->vp_bus_type, 0, sizeof(vp_bus_type));
> +
> +	for (i = 0; i < num_vps; i++) {
> +		/*
> +		 * Find corresponding vp bus type.
> +		 */
> +		vp_idx = get_vp_idx_from_vp(shared_mode_feat->vp_name[i]);
> +		shared_mode_feat->vp_bus_type[i] = vp_bus_type[vp_idx];
> +	}
> +}
> +
> +static int dispc_update_shared_mode_features(struct dispc_features *shared_mode_feat,
> +					     struct dispc_device *dispc)
> +{
> +	int r = 0, i = 0;
> +
> +	dev_dbg(dispc->dev, "Start updating dispc feature list for shared mode:\n");
> +
> +	/*
> +	 * Start with a shallow copy from existing features and prune the list
> +	 * as per what is actually made available to Linux
> +	 */
> +	memcpy((void *)shared_mode_feat, (void *)dispc->feat, sizeof(*shared_mode_feat));
> +	shared_mode_feat->num_vps = device_property_string_array_count(dispc->dev,
> +								       "ti,dss-shared-mode-vp");
> +	shared_mode_feat->num_planes = device_property_string_array_count(dispc->dev,
> +									  "ti,dss-shared-mode-planes");
> +
> +	r = device_property_read_string(dispc->dev, "ti,dss-shared-mode-common",
> +					(const char **)&shared_mode_feat->common);
> +	if (r) {
> +		dev_err(dispc->dev, "failed to read shared video port name: %d\n", r);
> +		return r;
> +	}
> +
> +	memset(shared_mode_feat->vid_name, 0, sizeof(shared_mode_feat->vid_name));
> +	r = device_property_read_string_array(dispc->dev, "ti,dss-shared-mode-planes",
> +					      shared_mode_feat->vid_name, TIDSS_MAX_PLANES);
> +	if (r < 0) {
> +		dev_err(dispc->dev, "failed to read client vid layer name: %d\n", r);
> +		return r;
> +	}
> +
> +	r = device_property_read_u32_array(dispc->dev, "ti,dss-shared-mode-vp-owned",
> +					   dispc->tidss->shared_mode_owned_vps,
> +					   shared_mode_feat->num_vps);
> +	if (r < 0) {
> +		dev_err(dispc->dev, "failed to read owned vp list: %d\n", r);
> +		return r;
> +	}
> +
> +	memset(shared_mode_feat->vp_name, 0, sizeof(shared_mode_feat->vp_name));
> +	r = device_property_read_string_array(dispc->dev, "ti,dss-shared-mode-vp",
> +					      shared_mode_feat->vp_name, TIDSS_MAX_PORTS);
> +	if (r < 0) {
> +		dev_err(dispc->dev, "failed to read shared video port name: %d\n", r);
> +		return r;
> +	}
> +
> +	memset(shared_mode_feat->vid_order, 0, sizeof(shared_mode_feat->vid_order));
> +	r = device_property_read_u32_array(dispc->dev, "ti,dss-shared-mode-plane-zorder",
> +					   shared_mode_feat->vid_order,
> +					   shared_mode_feat->num_planes);
> +	if (r < 0) {
> +		dev_err(dispc->dev, "failed to read vid_order array name: %d\n", r);
> +		return r;
> +	}
> +	memcpy((void *)shared_mode_feat->vpclk_name, (void *)shared_mode_feat->vp_name,
> +	       sizeof(shared_mode_feat->vpclk_name));
> +	memset(shared_mode_feat->ovr_name, 0, sizeof(shared_mode_feat->ovr_name));
> +
> +	for (i = 0; i < shared_mode_feat->num_vps; i++) {
> +		shared_mode_feat->ovr_name[i] = get_ovr_from_vp(shared_mode_feat->vp_name[i]);
> +		dev_dbg(dispc->dev, "vp[%d] = %s, ovr[%d] = %s vpclk[%d] = %s vp_owned[%d] = %d\n",
> +			i,  shared_mode_feat->vp_name[i], i, shared_mode_feat->ovr_name[i], i,
> +			shared_mode_feat->vpclk_name[i], i, dispc->tidss->shared_mode_owned_vps[i]);
> +	}
> +
> +	for (i = 0; i < shared_mode_feat->num_planes; i++) {
> +		if (!strncmp("vidl", shared_mode_feat->vid_name[i], 4))
> +			shared_mode_feat->vid_lite[i] = true;
> +		dev_dbg(dispc->dev, "vid[%d] = %s, vid_order[%d] = %u vid_lite[%d] = %u\n", i,
> +			shared_mode_feat->vid_name[i], i, shared_mode_feat->vid_order[i], i,
> +			shared_mode_feat->vid_lite[i]);
> +	}
> +
> +	if (!strcmp(shared_mode_feat->common, "common1"))
> +		shared_mode_feat->common_regs = tidss_am62_common1_regs;
> +
> +	dev_dbg(dispc->dev, "common : %s\n", shared_mode_feat->common);
> +	dispc_shared_mode_update_bus_type(shared_mode_feat, dispc);
> +	dev_dbg(dispc->dev, "Feature list updated for shared mode\n");
> +
> +	return 0;
> +}
> +
>   void dispc_vp_setup(struct dispc_device *dispc, u32 hw_videoport,
>   		    const struct drm_crtc_state *state, bool newmodeset)
>   {
> @@ -2662,6 +2834,16 @@ void dispc_vp_setup(struct dispc_device *dispc, u32 hw_videoport,
>   	dispc_vp_set_color_mgmt(dispc, hw_videoport, state, newmodeset);
>   }
>   
> +static bool dispc_owns_global_common_in_shared_mode(struct dispc_device *dispc)
> +{
> +	if ((!strcmp(dispc->feat->common, "common") ||
> +	     !strcmp(dispc->feat->common, "common_m")) &&
> +	     dispc->tidss->shared_mode)
> +		return true;
> +	else
> +		return false;
> +}
> +
>   int dispc_runtime_suspend(struct dispc_device *dispc)
>   {
>   	dev_dbg(dispc->dev, "suspend\n");
> @@ -2846,6 +3028,7 @@ int dispc_init(struct tidss_device *tidss)
>   	struct platform_device *pdev = to_platform_device(dev);
>   	struct dispc_device *dispc;
>   	const struct dispc_features *feat;
> +	struct dispc_features *shared_mode_feat;
>   	unsigned int i, num_fourccs;
>   	int r = 0;
>   
> @@ -2887,6 +3070,21 @@ int dispc_init(struct tidss_device *tidss)
>   
>   	dispc->num_fourccs = num_fourccs;
>   
> +	if (tidss->shared_mode) {
> +		dev_dbg(dev, "%s : DSS is being shared with remote core\n", __func__);
> +		shared_mode_feat = devm_kzalloc(dev, sizeof(*shared_mode_feat), GFP_KERNEL);
> +		if (!shared_mode_feat)
> +			return -ENOMEM;
> +
> +		r = dispc_update_shared_mode_features(shared_mode_feat, dispc);
> +		if (r)
> +			return r;
> +
> +		tidss->feat = (const struct dispc_features *)shared_mode_feat;
> +		feat = tidss->feat;
> +		dispc->feat = feat;
> +	}
> +
>   	dispc_common_regmap = dispc->feat->common_regs;
>   
>   	r = dispc_iomap_resource(pdev, dispc->feat->common,
> @@ -2933,25 +3131,37 @@ int dispc_init(struct tidss_device *tidss)
>   	}
>   
>   	if (feat->subrev == DISPC_AM65X) {
> -		r = dispc_init_am65x_oldi_io_ctrl(dev, dispc);
> -		if (r)
> -			return r;
> +		/*
> +		 * For shared mode, Initialize the OLDI IO control only if we own
> +		 * the OLDI Tx ports
> +		 */
> +		if (!tidss->shared_mode || tidss->shared_mode_own_oldi) {
> +			r = dispc_init_am65x_oldi_io_ctrl(dev, dispc);
> +			if (r)
> +				return r;
> +		}

I haven't studied this, but the shared_mode_own_oldi feels like very SoC 
specific. Will we need something similar on other SoC, maybe for other 
outputs too? Maybe this needs something more generic.

And it's not clear to me how to manage the whole pipeline. Let's say we 
have a board with DSS and an external bridge, like SiI9022, and a HDMI 
connector. I assume RTOS must control the SiI9022 and the connector 
(HPD, HDMI +5V line). Does that mean that the DT for Linux will be 
special in that case? Will we even have anything connected to the DSS 
port in the DT?

>   	}
>   
> -	dispc->fclk = devm_clk_get(dev, "fck");
> -	if (IS_ERR(dispc->fclk)) {
> -		dev_err(dev, "%s: Failed to get fclk: %ld\n",
> -			__func__, PTR_ERR(dispc->fclk));
> -		return PTR_ERR(dispc->fclk);
> -	}
> -	dev_dbg(dev, "DSS fclk %lu Hz\n", clk_get_rate(dispc->fclk));
> +	/*
> +	 * For shared mode, Initialize the hardware and clocking only if processing core running
> +	 * Linux has ownership of DSS global register space
> +	 */
> +	if (!tidss->shared_mode || dispc_owns_global_common_in_shared_mode(dispc)) {
> +		dispc->fclk = devm_clk_get(dev, "fck");
> +		if (IS_ERR(dispc->fclk)) {
> +			dev_err(dev, "%s: Failed to get fclk: %ld\n",
> +				__func__, PTR_ERR(dispc->fclk));
> +			return PTR_ERR(dispc->fclk);
> +		}
> +		dev_dbg(dev, "DSS fclk %lu Hz\n", clk_get_rate(dispc->fclk));
>   
> -	of_property_read_u32(dispc->dev->of_node, "max-memory-bandwidth",
> -			     &dispc->memory_bandwidth_limit);
> +		of_property_read_u32(dispc->dev->of_node, "max-memory-bandwidth",
> +				     &dispc->memory_bandwidth_limit);
>   
> -	r = dispc_init_hw(dispc);
> -	if (r)
> -		return r;
> +		r = dispc_init_hw(dispc);
> +		if (r)
> +			return r;
> +	}
>   
>   	tidss->dispc = dispc;
>   
> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.h b/drivers/gpu/drm/tidss/tidss_dispc.h
> index 086327d51a90..368a39941b34 100644
> --- a/drivers/gpu/drm/tidss/tidss_dispc.h
> +++ b/drivers/gpu/drm/tidss/tidss_dispc.h
> @@ -80,7 +80,7 @@ struct dispc_features {
>   	const char *vp_name[TIDSS_MAX_PORTS]; /* Should match dt reg names */
>   	const char *ovr_name[TIDSS_MAX_PORTS]; /* Should match dt reg names */
>   	const char *vpclk_name[TIDSS_MAX_PORTS]; /* Should match dt clk names */
> -	const enum dispc_vp_bus_type vp_bus_type[TIDSS_MAX_PORTS];
> +	enum dispc_vp_bus_type vp_bus_type[TIDSS_MAX_PORTS];
>   	struct tidss_vp_feat vp_feat;
>   	u32 num_planes;
>   	const char *vid_name[TIDSS_MAX_PLANES]; /* Should match dt reg names */
> diff --git a/drivers/gpu/drm/tidss/tidss_drv.c b/drivers/gpu/drm/tidss/tidss_drv.c
> index d15f836dca95..141481635578 100644
> --- a/drivers/gpu/drm/tidss/tidss_drv.c
> +++ b/drivers/gpu/drm/tidss/tidss_drv.c
> @@ -32,6 +32,10 @@ int tidss_runtime_get(struct tidss_device *tidss)
>   
>   	dev_dbg(tidss->dev, "%s\n", __func__);
>   
> +	/* No PM in display sharing mode */
> +	if (tidss->shared_mode)
> +		return 0;
> +

I'm not sure how to implement it, but I think it would be much nicer if 
the PM code in the driver stayed at least mostly the same. The driver 
can't suspend the hardware, but the meaning of the pm function calls 
would still make sense, i.e. pm_runtime_get() means that tidss wants the 
hardware to be enabled. If the HW already is enabled (is it would be as 
RTOS controls it), nothing would happen.

>   	r = pm_runtime_resume_and_get(tidss->dev);
>   	WARN_ON(r < 0);
>   	return r;
> @@ -43,6 +47,9 @@ void tidss_runtime_put(struct tidss_device *tidss)
>   
>   	dev_dbg(tidss->dev, "%s\n", __func__);
>   
> +	if (tidss->shared_mode)
> +		return;
> +
>   	pm_runtime_mark_last_busy(tidss->dev);
>   
>   	r = pm_runtime_put_autosuspend(tidss->dev);
> @@ -140,21 +147,23 @@ static int tidss_probe(struct platform_device *pdev)
>   
>   	spin_lock_init(&tidss->wait_lock);
>   
> +	tidss->shared_mode = device_property_read_bool(dev, "ti,dss-shared-mode");
>   	ret = dispc_init(tidss);
>   	if (ret) {
>   		dev_err(dev, "failed to initialize dispc: %d\n", ret);
>   		return ret;
>   	}
>   
> -	pm_runtime_enable(dev);
> -
> -	pm_runtime_set_autosuspend_delay(dev, 1000);
> -	pm_runtime_use_autosuspend(dev);
> +	if (!tidss->shared_mode) {
> +		pm_runtime_enable(dev);
> +		pm_runtime_set_autosuspend_delay(dev, 1000);
> +		pm_runtime_use_autosuspend(dev);
>   
>   #ifndef CONFIG_PM
> -	/* If we don't have PM, we need to call resume manually */
> -	dispc_runtime_resume(tidss->dispc);
> +		/* If we don't have PM, we need to call resume manually */
> +		dispc_runtime_resume(tidss->dispc);
>   #endif
> +	}
>   
>   	ret = tidss_modeset_init(tidss);
>   	if (ret < 0) {
> @@ -196,6 +205,8 @@ static int tidss_probe(struct platform_device *pdev)
>   	tidss_irq_uninstall(ddev);
>   
>   err_runtime_suspend:
> +	if (tidss->shared_mode)
> +		return ret;
>   #ifndef CONFIG_PM
>   	dispc_runtime_suspend(tidss->dispc);
>   #endif
> @@ -219,12 +230,14 @@ static void tidss_remove(struct platform_device *pdev)
>   
>   	tidss_irq_uninstall(ddev);
>   
> +	if (!tidss->shared_mode) {
>   #ifndef CONFIG_PM
> -	/* If we don't have PM, we need to call suspend manually */
> -	dispc_runtime_suspend(tidss->dispc);
> +		/* If we don't have PM, we need to call suspend manually */
> +		dispc_runtime_suspend(tidss->dispc);
>   #endif
> -	pm_runtime_dont_use_autosuspend(dev);
> -	pm_runtime_disable(dev);
> +		pm_runtime_dont_use_autosuspend(dev);
> +		pm_runtime_disable(dev);
> +	}
>   
>   	/* devm allocated dispc goes away with the dev so mark it NULL */
>   	dispc_remove(tidss);
> diff --git a/drivers/gpu/drm/tidss/tidss_drv.h b/drivers/gpu/drm/tidss/tidss_drv.h
> index d7f27b0b0315..68d53c70651d 100644
> --- a/drivers/gpu/drm/tidss/tidss_drv.h
> +++ b/drivers/gpu/drm/tidss/tidss_drv.h
> @@ -31,6 +31,12 @@ struct tidss_device {
>   
>   	spinlock_t wait_lock;	/* protects the irq masks */
>   	dispc_irq_t irq_mask;	/* enabled irqs in addition to wait_list */
> +
> +	bool shared_mode; /* DSS resources shared between remote core and Linux */
> +
> +	/* 1: VP owned by Linux 0: VP is owned by remote and shared with Linux */
> +	u32 shared_mode_owned_vps[TIDSS_MAX_PORTS];

Boolean, I think?

> +	bool shared_mode_own_oldi; /* Linux needs to configure OLDI in shared mode */
>   };
>   
>   #define to_tidss(__dev) container_of(__dev, struct tidss_device, ddev)

Re: [RFC PATCH 3/3] arm64: dts: ti: k3-am62x: Add overlay to use DSS in display sharing mode

[Tomi Valkeinen]

Hi,

On 16/01/2024 15:41, Devarsh Thakkar wrote:
> This overlay needs to be used with display sharing supported device
> manager firmware only.
> 
> Remote core running this firmware has write access to "common" register
> space, VIDL pipeline, OVR1 overlay and VP1 videoport.
> 
> The processing core running Linux is provided write access to VID
> pipeline and "common1" register space.
> 
> The VP1 video port is shared between Linux and remote core with remote
> core configuring the overlay manager to set Zorder 1 for VID pipeline
> and Zorder 2 for VIDL pipeline.
> 
> Add reserved memory region for framebuffer region used by remote core in
> dss shared mode overlay file so that Linux does not re-use the same
> while allocating memory.

I don't understand this one. Why is RAM used by RTOS accessible by Linux 
in the first place?

  Tomi

> Also add a label for reserved memory region in base device-tree file so
> that it can be referred back in overlay file.
> 
> Signed-off-by: Devarsh Thakkar <devarsht@ti.com>
> ---
>   arch/arm64/boot/dts/ti/Makefile               |  1 +
>   .../arm64/boot/dts/ti/k3-am62x-sk-common.dtsi |  2 +-
>   .../dts/ti/k3-am62x-sk-dss-shared-mode.dtso   | 33 +++++++++++++++++++
>   3 files changed, 35 insertions(+), 1 deletion(-)
>   create mode 100644 arch/arm64/boot/dts/ti/k3-am62x-sk-dss-shared-mode.dtso
> 
> diff --git a/arch/arm64/boot/dts/ti/Makefile b/arch/arm64/boot/dts/ti/Makefile
> index 52c1dc910308..ff832741b367 100644
> --- a/arch/arm64/boot/dts/ti/Makefile
> +++ b/arch/arm64/boot/dts/ti/Makefile
> @@ -35,6 +35,7 @@ dtb-$(CONFIG_ARCH_K3) += k3-am62x-sk-csi2-ov5640.dtbo
>   dtb-$(CONFIG_ARCH_K3) += k3-am62x-sk-csi2-tevi-ov5640.dtbo
>   dtb-$(CONFIG_ARCH_K3) += k3-am62x-sk-csi2-imx219.dtbo
>   dtb-$(CONFIG_ARCH_K3) += k3-am62x-sk-hdmi-audio.dtbo
> +dtb-$(CONFIG_ARCH_K3) += k3-am62x-sk-dss-shared-mode.dtbo
>   
>   # Boards with AM64x SoC
>   dtb-$(CONFIG_ARCH_K3) += k3-am642-evm.dtb
> diff --git a/arch/arm64/boot/dts/ti/k3-am62x-sk-common.dtsi b/arch/arm64/boot/dts/ti/k3-am62x-sk-common.dtsi
> index 33768c02d8eb..8b55ca10102f 100644
> --- a/arch/arm64/boot/dts/ti/k3-am62x-sk-common.dtsi
> +++ b/arch/arm64/boot/dts/ti/k3-am62x-sk-common.dtsi
> @@ -34,7 +34,7 @@ memory@80000000 {
>   		reg = <0x00000000 0x80000000 0x00000000 0x80000000>;
>   	};
>   
> -	reserved-memory {
> +	reserved_memory: reserved-memory {
>   		#address-cells = <2>;
>   		#size-cells = <2>;
>   		ranges;
> diff --git a/arch/arm64/boot/dts/ti/k3-am62x-sk-dss-shared-mode.dtso b/arch/arm64/boot/dts/ti/k3-am62x-sk-dss-shared-mode.dtso
> new file mode 100644
> index 000000000000..02153748a5c2
> --- /dev/null
> +++ b/arch/arm64/boot/dts/ti/k3-am62x-sk-dss-shared-mode.dtso
> @@ -0,0 +1,33 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/**
> + * DT overlay to enable display sharing mode for AM62P DSS0
> + * This is compatible with custom AM62 Device Manager firmware
> + *
> + * Copyright (C) 2023 Texas Instruments Incorporated - http://www.ti.com/
> + */
> +
> +/dts-v1/;
> +/plugin/;
> +
> +#include <dt-bindings/interrupt-controller/irq.h>
> +#include <dt-bindings/interrupt-controller/arm-gic.h>
> +
> +&dss0 {
> +	ti,dss-shared-mode;
> +	ti,dss-shared-mode-vp = "vp1";
> +	ti,dss-shared-mode-vp-owned = <0>;
> +	ti,dss-shared-mode-common = "common1";
> +	ti,dss-shared-mode-planes = "vid";
> +	ti,dss-shared-mode-plane-zorder = <0>;
> +	interrupts = <GIC_SPI 85 IRQ_TYPE_LEVEL_HIGH>;
> +};
> +
> +&reserved_memory {
> +	#address-cells = <2>;
> +	#size-cells = <2>;
> +	rtos_framebuffer_memory_region: rtos-framebuffer-memory@94800000 {
> +		compatible = "shared-dma-pool";
> +		reg = <0x00 0x94800000 0x00 0x08000000>;
> +		no-map;
> +	};
> +};

Re: [RFC PATCH 2/3] drm/tidss: Add support for display sharing

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2646 bytes --]

Hi,

Thanks a lot for working on that.

On Tue, Jan 16, 2024 at 07:11:41PM +0530, Devarsh Thakkar wrote:
> Display subsystem present in TI Keystone family of devices supports sharing
> of display between multiple hosts as it provides separate register space
> (common* region) for each host to programming display controller and also a
> unique interrupt line for each host.
> 
> This adds support for display sharing, by allowing partitioning of
> resources either at video port level or at video plane level as
> described below :
> 
> 1) Linux can own (i.e have write access) completely one or more of video
> ports along with corresponding resources (viz. overlay managers,
> video planes) used by Linux in context of those video ports.
> Even if Linux is owning
> these video ports it can still share this video port with a remote core
> which can own one or more video planes associated with this video port.
> 
> 2) Linux owns one or more of the video planes with video port
> (along with corresponding overlay manager) associated with these planes
> being owned and controlled by a remote core. Linux still has read-only
> access to the associated video port and overlay managers so that it can
> parse the settings made by remote core.

So, just to make sure we're on the same page. 1) means Linux drives the
whole display engine, but can lend planes to the R5? How does that work,
is Linux aware of the workload being there (plane size, format, etc) ?

And 2) would mean that the display engine is under the R5 control and
Linux only gets to fill the plane and let the firmware know of what it
wants?

If so, do we even need the tidss driver in the second case? We could
just write a fwkms driver of some sorts that could be used by multiple
implementations of the same "defer to firmware" logic.

> For both the cases, the resources used in context of processing core
> running Linux along with ownership information are exposed by user as
> part of device-tree blob and driver uses an updated feature list tailored
> for this shared mode accordingly. The driver also auto-populates
> matching overlay managers and output types from shared video
> port list provided in device-tree blob.
> In dispc_feature struct remove const access specfier for output_type
> array as it is required to be updated dynamically in run-time for shared
> mode.

I'm also not entirely sure that the device tree is the right path there.
Surely the firmware capabilities will evolve over time, while the device
tree won't. Is there some way to make it discoverable at probe time by
the driver?

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [RFC PATCH 1/3] dt-bindings: display: ti,am65x-dss: Add support for display sharing mode

[Devarsh Thakkar]

Hi Rob,

Thanks for the review.

On 19/01/24 19:25, Rob Herring wrote:
> On Thu, Jan 18, 2024 at 7:59 AM Devarsh Thakkar <devarsht@ti.com> wrote:
>>
>> Hi Rob,
>>
>> Thanks for the quick review.
>>
>> On 18/01/24 01:43, Rob Herring wrote:
>>> On Tue, Jan 16, 2024 at 07:11:40PM +0530, Devarsh Thakkar wrote:
>>>> Add support for using TI Keystone DSS hardware present in display
>>>> sharing mode.
>>>>
>>>> TI Keystone DSS hardware supports partitioning of resources between
>>>> multiple hosts as it provides separate register space and unique
>>>> interrupt line to each host.
>>>>
>>>> The DSS hardware can be used in shared mode in such a way that one or
>>>> more of video planes can be owned by Linux wherease other planes can be
>>>> owned by remote cores.
>>>>
>>>> One or more of the video ports can be dedicated exclusively to a
>>>> processing core, wherease some of the video ports can be shared between
>>>> two hosts too with only one of them having write access.
>>>>
>>>> Signed-off-by: Devarsh Thakkar <devarsht@ti.com>
>>>> ---
>>>>  .../bindings/display/ti/ti,am65x-dss.yaml     | 82 +++++++++++++++++++
>>>>  1 file changed, 82 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml b/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml
>>>> index 55e3e490d0e6..d9bc69fbf1fb 100644
>>>> --- a/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml
>>>> +++ b/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml
>>>> @@ -112,6 +112,86 @@ properties:
>>>>        Input memory (from main memory to dispc) bandwidth limit in
>>>>        bytes per second
>>>>
>>>> +  ti,dss-shared-mode:
>>>> +    type: boolean
>>>> +    description:
>>>> +      TI DSS7 supports sharing of display between multiple hosts
>>>> +      as it provides separate register space for display configuration and
>>>> +      unique interrupt line to each host.
>>>
>>> If you care about line breaks, you need '|'.
>>>
>>
>> Noted.
>>
>>>> +      One of the host is provided access to the global display
>>>> +      configuration labelled as "common" region of DSS allows that host
>>>> +      exclusive access to global registers of DSS while other host can
>>>> +      configure the display for it's usage using a separate register
>>>> +      space labelled as "common1".
>>>> +      The DSS resources can be partitioned in such a way that one or more
>>>> +      of the video planes are owned by Linux whereas other video planes
>>>
>>> Your h/w can only run Linux?
>>>
>>> What if you want to use this same binding to define the configuration to
>>> the 'remote processor'? You can easily s/Linux/the OS/, but it all
>>> should be reworded to describe things in terms of the local processor.
>>>
>>
>> It can run both Linux and RTOS or for that matter any other OS too. But yes I
>> got your point, will reword accordingly.
>>
>>>> +      can be owned by a remote core.
>>>> +      The video port controlling these planes acts as a shared video port
>>>> +      and it can be configured with write access either by Linux or the
>>>> +      remote core in which case Linux only has read-only access to that
>>>> +      video port.
>>>
>>> What is the purpose of this property when all the other properties are
>>> required?
>>>
>>
>> The ti,dss-shared-mode and below group of properties are optional. But
>> if ti,dss-shared-mode is set then only driver should parse below set of
>> properties.
> 
> Let me re-phrase. Drop this property. It serves no purpose since all
> the properties have to be present anyway.
> 

Noted.

>>>> +  ti,dss-shared-mode-planes:
>>>> +    description:
>>>> +      The video layer that is owned by processing core running Linux.
>>>> +      The display driver running from Linux has exclusive write access to
>>>> +      this video layer.
>>>> +    $ref: /schemas/types.yaml#/definitions/string
>>>> +    enum: [vidl, vid]
>>>> +
>>>> +  ti,dss-shared-mode-vp:
>>>> +    description:
>>>> +      The video port that is being used in context of processing core
>>>> +      running Linux with display susbsytem being used in shared mode.
>>>> +      This can be owned either by the processing core running Linux in
>>>> +      which case Linux has the write access and the responsibility to
>>>> +      configure this video port and the associated overlay manager or
>>>> +      it can be shared between core running Linux and a remote core
>>>> +      with remote core provided with write access to this video port and
>>>> +      associated overlay managers and remote core configures and drives
>>>> +      this video port also feeding data from one or more of the
>>>> +      video planes owned by Linux, with Linux only having read-only access
>>>> +      to this video port and associated overlay managers.
>>>> +
>>>> +    $ref: /schemas/types.yaml#/definitions/string
>>>> +    enum: [vp1, vp2]
>>>> +
>>>> +  ti,dss-shared-mode-common:
>>>> +    description:
>>>> +      The DSS register region owned by processing core running Linux.
>>>> +    $ref: /schemas/types.yaml#/definitions/string
>>>> +    enum: [common, common1]
>>>> +
>>>> +  ti,dss-shared-mode-vp-owned:
>>>> +    description:
>>>> +      This tells whether processing core running Linux has write access to
>>>> +      the video ports enlisted in ti,dss-shared-mode-vps.
>>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>>> +    enum: [0, 1]
>>>
>>> This can be boolean. Do writes abort or just get ignored? The latter can
>>> be probed and doesn't need a property.
>>>
>>
>> Although we have kept all these properties as enums, but actually in driver we
>> are treating them as array of enums and using device_property_read_u32_array.
>>
>> The reason being that for SoCs using am65x-dss bindings they can only have
>> single entry either vp1 or vp2 or 0 or 1 as there are only two video ports. So
>> for them the device tree overlay would look like :
>> &dss0 {
>>
>>         ti,dss-shared-mode;
>>
>>         ti,dss-shared-mode-vp = "vp1";
>>
>>         ti,dss-shared-mode-vp-owned = <0>;
>>
>>         ti,dss-shared-mode-common = "common1";
>>
>>         ti,dss-shared-mode-planes = "vid";
>>
>>         ti,dss-shared-mode-plane-zorder = <0>;
>>
>>         interrupts = <GIC_SPI 85 IRQ_TYPE_LEVEL_>;
>> }
>>
>> But we also plan to extend these bindings to SoCs using
>> Documentation/devicetree/bindings/display/ti/ti,j721e-dss.yaml where there are
>> multiple video ports. So in that the driver and bindings should support below
>> configuration :
> 
> What are you waiting for? In that case, all these properties need to
> be in a shared schema file and referenced here. Otherwise you will be
> defining their types twice (and different types too if some are
> changed to arrays).
> 

Noted, thanks for suggesting this, shared schema file indeed looks like
a better idea.

>> &dss0 {
>>
>>         ti,dss-shared-mode;
>>
>>         ti,dss-shared-mode-vp = "vp1 vp2";
>>
>>         ti,dss-shared-mode-vp-owned = <0 1>;
>>
>>         ti,dss-shared-mode-common = "common_s1";
>>
>>         ti,dss-shared-mode-planes = "vid1 vidl1";
>>
>>         ti,dss-shared-mode-plane-zorder = <0 1>;
>>
>>         interrupts = <GIC_SPI 85 IRQ_TYPE_LEVEL_>;
>> }
>>
>> As I am using device_property_read_u32_array in driver I thought to keep this
>> as uint32 in enum for am65x.yaml which works well with the driver.
> 
> The type and what accessor functions the kernel uses should match. I
> plan to add (debug) checking on this. Debug only because as there's no
> type info in the DTB, it all has to be extracted from schemas and put
> into the kernel.
> 

Yes, with the shared schema it should be array instead of integer.

>>>> +
>>>> +  ti,dss-shared-mode-plane-zorder:
>>>> +    description:
>>>> +      The zorder of the planes owned by Linux.
>>>> +      For the scenario where Linux is not having write access to associated
>>>> +      video port, this field is just for
>>>> +      informational purpose to enumerate the zorder configuration
>>>> +      being used by remote core.
>>>> +
>>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>>> +    enum: [0, 1]
>>>
>>> I don't understand how 0 or 1 defines Z-order.
>>>
>>
>> As there are only two planes in total so z-order can be either 0 or 1 for the
>> shared mode plane as there is only a single entry of plane.
>> For e.g. if ti,dss-shared-mode-plane-zorder is 1 then it means the plane owned
>> by Linux is programmed as topmost plane wherease the plane owned by remote
>> core is programmed as the underneath one.
> 
> Please reword the description to make all this clear. The index is the
> plane number and value is the z-order with 0 being bottom and N being
> the top. I guess this should be an array as well.
> 


Yes, this was kept as uint32 since with am65x-dss only one plane was
available for sharing, but with the shared schema this too should be an
array instead of integer.

Regards
Devarsh

> Rob
> 

Re: [RFC PATCH 3/3] arm64: dts: ti: k3-am62x: Add overlay to use DSS in display sharing mode

[Devarsh Thakkar]

Hi Tomi,

Thanks for the review.

On 23/01/24 13:59, Tomi Valkeinen wrote:
> Hi,
> 
> On 16/01/2024 15:41, Devarsh Thakkar wrote:
>> This overlay needs to be used with display sharing supported device
>> manager firmware only.
>>
>> Remote core running this firmware has write access to "common" register
>> space, VIDL pipeline, OVR1 overlay and VP1 videoport.
>>
>> The processing core running Linux is provided write access to VID
>> pipeline and "common1" register space.
>>
>> The VP1 video port is shared between Linux and remote core with remote
>> core configuring the overlay manager to set Zorder 1 for VID pipeline
>> and Zorder 2 for VIDL pipeline.
>>
>> Add reserved memory region for framebuffer region used by remote core in
>> dss shared mode overlay file so that Linux does not re-use the same
>> while allocating memory.
> 
> I don't understand this one. Why is RAM used by RTOS accessible by Linux
> in the first place?
> 

Well, I think the R5 SPL initializes full DDR before starting firmwares
on remote cores and the regions used by this remote cores be it for IPC
or Code/data are marked as reserved both in Linux as well as U-boot so
that Linux/U-boot does not use it [1].

Same scheme is followed here w.r. t RTOS framebuffer too.

[1] :
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/ti/k3-am62x-sk-common.dtsi?h=v6.7#n63

Regards
Devarsh

>  Tomi
> 
>> Also add a label for reserved memory region in base device-tree file so
>> that it can be referred back in overlay file.
>>
>> Signed-off-by: Devarsh Thakkar <devarsht@ti.com>
>> ---
>>   arch/arm64/boot/dts/ti/Makefile               |  1 +
>>   .../arm64/boot/dts/ti/k3-am62x-sk-common.dtsi |  2 +-
>>   .../dts/ti/k3-am62x-sk-dss-shared-mode.dtso   | 33 +++++++++++++++++++
>>   3 files changed, 35 insertions(+), 1 deletion(-)
>>   create mode 100644
>> arch/arm64/boot/dts/ti/k3-am62x-sk-dss-shared-mode.dtso
>>
>> diff --git a/arch/arm64/boot/dts/ti/Makefile
>> b/arch/arm64/boot/dts/ti/Makefile
>> index 52c1dc910308..ff832741b367 100644
>> --- a/arch/arm64/boot/dts/ti/Makefile
>> +++ b/arch/arm64/boot/dts/ti/Makefile
>> @@ -35,6 +35,7 @@ dtb-$(CONFIG_ARCH_K3) += k3-am62x-sk-csi2-ov5640.dtbo
>>   dtb-$(CONFIG_ARCH_K3) += k3-am62x-sk-csi2-tevi-ov5640.dtbo
>>   dtb-$(CONFIG_ARCH_K3) += k3-am62x-sk-csi2-imx219.dtbo
>>   dtb-$(CONFIG_ARCH_K3) += k3-am62x-sk-hdmi-audio.dtbo
>> +dtb-$(CONFIG_ARCH_K3) += k3-am62x-sk-dss-shared-mode.dtbo
>>     # Boards with AM64x SoC
>>   dtb-$(CONFIG_ARCH_K3) += k3-am642-evm.dtb
>> diff --git a/arch/arm64/boot/dts/ti/k3-am62x-sk-common.dtsi
>> b/arch/arm64/boot/dts/ti/k3-am62x-sk-common.dtsi
>> index 33768c02d8eb..8b55ca10102f 100644
>> --- a/arch/arm64/boot/dts/ti/k3-am62x-sk-common.dtsi
>> +++ b/arch/arm64/boot/dts/ti/k3-am62x-sk-common.dtsi
>> @@ -34,7 +34,7 @@ memory@80000000 {
>>           reg = <0x00000000 0x80000000 0x00000000 0x80000000>;
>>       };
>>   -    reserved-memory {
>> +    reserved_memory: reserved-memory {
>>           #address-cells = <2>;
>>           #size-cells = <2>;
>>           ranges;
>> diff --git a/arch/arm64/boot/dts/ti/k3-am62x-sk-dss-shared-mode.dtso
>> b/arch/arm64/boot/dts/ti/k3-am62x-sk-dss-shared-mode.dtso
>> new file mode 100644
>> index 000000000000..02153748a5c2
>> --- /dev/null
>> +++ b/arch/arm64/boot/dts/ti/k3-am62x-sk-dss-shared-mode.dtso
>> @@ -0,0 +1,33 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/**
>> + * DT overlay to enable display sharing mode for AM62P DSS0
>> + * This is compatible with custom AM62 Device Manager firmware
>> + *
>> + * Copyright (C) 2023 Texas Instruments Incorporated -
>> http://www.ti.com/
>> + */
>> +
>> +/dts-v1/;
>> +/plugin/;
>> +
>> +#include <dt-bindings/interrupt-controller/irq.h>
>> +#include <dt-bindings/interrupt-controller/arm-gic.h>
>> +
>> +&dss0 {
>> +    ti,dss-shared-mode;
>> +    ti,dss-shared-mode-vp = "vp1";
>> +    ti,dss-shared-mode-vp-owned = <0>;
>> +    ti,dss-shared-mode-common = "common1";
>> +    ti,dss-shared-mode-planes = "vid";
>> +    ti,dss-shared-mode-plane-zorder = <0>;
>> +    interrupts = <GIC_SPI 85 IRQ_TYPE_LEVEL_HIGH>;
>> +};
>> +
>> +&reserved_memory {
>> +    #address-cells = <2>;
>> +    #size-cells = <2>;
>> +    rtos_framebuffer_memory_region: rtos-framebuffer-memory@94800000 {
>> +        compatible = "shared-dma-pool";
>> +        reg = <0x00 0x94800000 0x00 0x08000000>;
>> +        no-map;
>> +    };
>> +};
> 
> 

Re: [RFC PATCH 2/3] drm/tidss: Add support for display sharing

[Devarsh Thakkar]

Hi Maxime,

Thanks a lot for checking on this.

On 26/01/24 17:45, Maxime Ripard wrote:
> Hi,
> 
> Thanks a lot for working on that.
> 
> On Tue, Jan 16, 2024 at 07:11:41PM +0530, Devarsh Thakkar wrote:
>> Display subsystem present in TI Keystone family of devices supports sharing
>> of display between multiple hosts as it provides separate register space
>> (common* region) for each host to programming display controller and also a
>> unique interrupt line for each host.
>>
>> This adds support for display sharing, by allowing partitioning of
>> resources either at video port level or at video plane level as
>> described below :
>>
>> 1) Linux can own (i.e have write access) completely one or more of video
>> ports along with corresponding resources (viz. overlay managers,
>> video planes) used by Linux in context of those video ports.
>> Even if Linux is owning
>> these video ports it can still share this video port with a remote core
>> which can own one or more video planes associated with this video port.
>>
>> 2) Linux owns one or more of the video planes with video port
>> (along with corresponding overlay manager) associated with these planes
>> being owned and controlled by a remote core. Linux still has read-only
>> access to the associated video port and overlay managers so that it can
>> parse the settings made by remote core.
> 
> So, just to make sure we're on the same page. 1) means Linux drives the
> whole display engine, but can lend planes to the R5? How does that work,
> is Linux aware of the workload being there (plane size, format, etc) ?
> 

Well, there is no dynamic procedure being followed for lending. The
partitioning scheme is decided and known before hand, and the remote
core firmware updated and compiled accordingly, and similarly the
device-tree overlay for Linux is also updated with partitioning
information before bootup.

What would happen here is that Linux will know before-hand this
partitioning information via device-tree properties and won't enumerate
the plane owned by RTOS, but it will enumerate the rest of the display
components and initialize the DSS, after which user can load the DSS
firmware on remote core and this firmware will only have control of
plane as it was compiled with that configuration.


> And 2) would mean that the display engine is under the R5 control and
> Linux only gets to fill the plane and let the firmware know of what it
> wants?
> 

Here too the partitioning information is pre-decided and remote core
firmware and device-tree overlay for Linux updated accordingly. But in
this case as remote core firmware owns the display (minus the plane
owned by Linux) it is started and initialized during the bootloader
phase itself where it initializes the DSS and starts rendering using the
plane owned by it and Linux just latches to the DSS without
re-initializing it, with write access only to the plane that is owned by
Linux. You can refer [1] for more details on this.


> If so, do we even need the tidss driver in the second case? We could
> just write a fwkms driver of some sorts that could be used by multiple
> implementations of the same "defer to firmware" logic.
> 

This feature of static partitioning of DSS resources is specific to DSS7
hardware (which is controlled by tidss driver) which supports dedicated
register space and interrupt line for each of the hosts [0], so that
multiple hosts can drive the display controller simultaneously as  per
the desired static partitioning of resources, and so I don't think a
separate driver is required here and tidss seems the right place to
support this, where using this device-tree approach different resource
partitioning schemas can be achieved as described here [1]. This was
also aligned with Tomi too where we discussed that tidss is the right
place to support this as we are simply leveraging the DSS hardware
capabilities of static partitioning here.


>> For both the cases, the resources used in context of processing core
>> running Linux along with ownership information are exposed by user as
>> part of device-tree blob and driver uses an updated feature list tailored
>> for this shared mode accordingly. The driver also auto-populates
>> matching overlay managers and output types from shared video
>> port list provided in device-tree blob.
>> In dispc_feature struct remove const access specfier for output_type
>> array as it is required to be updated dynamically in run-time for shared
>> mode.
> 
> I'm also not entirely sure that the device tree is the right path there.
> Surely the firmware capabilities will evolve over time, while the device
> tree won't. Is there some way to make it discoverable at probe time by
> the driver?
> 

I think the main highlight of the sharing feature is the hardware
capability where each host is provided separate irq and register space
to program display for their display context independently and without
any sort of inter-processor communication and the current implementation
just mirrors by specifying the display context information in the form
of device-tree properties instead of relying on any inter-processor
communication or negotiation phase.

Having said that, for the second case Linux still has read access to
display registers which can be read to infer some of the display
configuration set by the RTOS and we are infact using that to infer some
of the context like for e.g. what was the display mode set by RTOS, but
we don't want to rely completely on register reads of RTOS programmed
registers to gather full information regarding partitioning or even to
infer that we are in sharing mode.

The remote core firmware is compiled with the partitioning information
known before-hand and same is fed in the form of DT properties to Linux
and this looks much cleaner, simple and reliable to us.

But yes we wanted to support firmwares with different partitioning
schemes/capabilities as you said and so the tidss-shared-mode
device-tree properties were designed keeping in mind that they should be
adequate and flexible enough to define all different partition schemes
which can be supported, this is also demonstrated with an example here [1]

[0]:
https://software-dl.ti.com/processor-sdk-linux/esd/AM62PX/09_01_00_08/exports/docs/linux/Foundational_Components/Kernel/Kernel_Drivers/Display/DSS7.html#soc-hardware-description


[1]:
https://software-dl.ti.com/processor-sdk-linux/esd/AM62PX/09_01_00_08/exports/docs/linux/How_to_Guides/Target/How_to_enable_display_sharing_between_remotecore_and_Linux.html


Regards
Devarsh


> Maxime

Re: Re: [RFC PATCH 2/3] drm/tidss: Add support for display sharing

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 6347 bytes --]

Hi Devarsh,

On Thu, Feb 08, 2024 at 06:26:17PM +0530, Devarsh Thakkar wrote:
> Hi Maxime,
> 
> Thanks a lot for checking on this.
> 
> On 26/01/24 17:45, Maxime Ripard wrote:
> > Hi,
> > 
> > Thanks a lot for working on that.
> > 
> > On Tue, Jan 16, 2024 at 07:11:41PM +0530, Devarsh Thakkar wrote:
> >> Display subsystem present in TI Keystone family of devices supports sharing
> >> of display between multiple hosts as it provides separate register space
> >> (common* region) for each host to programming display controller and also a
> >> unique interrupt line for each host.
> >>
> >> This adds support for display sharing, by allowing partitioning of
> >> resources either at video port level or at video plane level as
> >> described below :
> >>
> >> 1) Linux can own (i.e have write access) completely one or more of video
> >> ports along with corresponding resources (viz. overlay managers,
> >> video planes) used by Linux in context of those video ports.
> >> Even if Linux is owning
> >> these video ports it can still share this video port with a remote core
> >> which can own one or more video planes associated with this video port.
> >>
> >> 2) Linux owns one or more of the video planes with video port
> >> (along with corresponding overlay manager) associated with these planes
> >> being owned and controlled by a remote core. Linux still has read-only
> >> access to the associated video port and overlay managers so that it can
> >> parse the settings made by remote core.
> > 
> > So, just to make sure we're on the same page. 1) means Linux drives the
> > whole display engine, but can lend planes to the R5? How does that work,
> > is Linux aware of the workload being there (plane size, format, etc) ?
> > 
> 
> Well, there is no dynamic procedure being followed for lending. The
> partitioning scheme is decided and known before hand, and the remote
> core firmware updated and compiled accordingly, and similarly the
> device-tree overlay for Linux is also updated with partitioning
> information before bootup.
> 
> What would happen here is that Linux will know before-hand this
> partitioning information via device-tree properties and won't enumerate
> the plane owned by RTOS, but it will enumerate the rest of the display
> components and initialize the DSS, after which user can load the DSS
> firmware on remote core and this firmware will only have control of
> plane as it was compiled with that configuration.

Right. If the RTOS is in control of a single plane, how it is expected
to deal with Linux shutting the CRTC down, or enforcing a configuration
that isn't compatible with what the RTOS expects (like a plane with a
higher zpos masking its plane), what is the mechanism to reconcile it?

> > And 2) would mean that the display engine is under the R5 control and
> > Linux only gets to fill the plane and let the firmware know of what it
> > wants?
> > 
> 
> Here too the partitioning information is pre-decided and remote core
> firmware and device-tree overlay for Linux updated accordingly. But in
> this case as remote core firmware owns the display (minus the plane
> owned by Linux) it is started and initialized during the bootloader
> phase itself where it initializes the DSS and starts rendering using the
> plane owned by it and Linux just latches to the DSS without
> re-initializing it, with write access only to the plane that is owned by
> Linux. You can refer [1] for more details on this.
>
> > If so, do we even need the tidss driver in the second case? We could
> > just write a fwkms driver of some sorts that could be used by multiple
> > implementations of the same "defer to firmware" logic.
> > 
> 
> This feature of static partitioning of DSS resources is specific to DSS7
> hardware (which is controlled by tidss driver) which supports dedicated
> register space and interrupt line for each of the hosts [0], so that
> multiple hosts can drive the display controller simultaneously as  per
> the desired static partitioning of resources, and so I don't think a
> separate driver is required here and tidss seems the right place to
> support this, where using this device-tree approach different resource
> partitioning schemas can be achieved as described here [1]. This was
> also aligned with Tomi too where we discussed that tidss is the right
> place to support this as we are simply leveraging the DSS hardware
> capabilities of static partitioning here.

If the only thing tidss does in the "owned by RTOS" is forwarding KMS
atomic states to the RTOS, then I'm still not sure why we need to
involve tidss at all.

It's not just about interrupts, it's also about how your arbitrate
between what Linux wants and what the RTOS wants. Like if the RTOS still
wants to output something but Linux wants to disable it, how do you
reconcile the two?

You have to have something that reconciles both, and typically for
firmware-based setup this will be the firmware's job.

That's very similar to what the RaspberryPi did with fkms, and I believe
that having a generic KMS-on-remoteproc driver when the firmware has
control over the display is the path forward.

> >> For both the cases, the resources used in context of processing core
> >> running Linux along with ownership information are exposed by user as
> >> part of device-tree blob and driver uses an updated feature list tailored
> >> for this shared mode accordingly. The driver also auto-populates
> >> matching overlay managers and output types from shared video
> >> port list provided in device-tree blob.
> >> In dispc_feature struct remove const access specfier for output_type
> >> array as it is required to be updated dynamically in run-time for shared
> >> mode.
> > 
> > I'm also not entirely sure that the device tree is the right path there.
> > Surely the firmware capabilities will evolve over time, while the device
> > tree won't. Is there some way to make it discoverable at probe time by
> > the driver?
>
> I think the main highlight of the sharing feature is the hardware
> capability where each host is provided separate irq and register space
> to program display for their display context independently

Wait, what do you mean by display context here?

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [RFC PATCH 2/3] drm/tidss: Add support for display sharing

[Devarsh Thakkar]

Hi Maxime,

Thanks for the quick reply.

On 13/02/24 19:34, Maxime Ripard wrote:
> Hi Devarsh,
> 
> On Thu, Feb 08, 2024 at 06:26:17PM +0530, Devarsh Thakkar wrote:
>> Hi Maxime,
>>
>> Thanks a lot for checking on this.
>>
>> On 26/01/24 17:45, Maxime Ripard wrote:
>>> Hi,
>>>
>>> Thanks a lot for working on that.
>>>
>>> On Tue, Jan 16, 2024 at 07:11:41PM +0530, Devarsh Thakkar wrote:
>>>> Display subsystem present in TI Keystone family of devices supports sharing
>>>> of display between multiple hosts as it provides separate register space
>>>> (common* region) for each host to programming display controller and also a
>>>> unique interrupt line for each host.
>>>>
>>>> This adds support for display sharing, by allowing partitioning of
>>>> resources either at video port level or at video plane level as
>>>> described below :
>>>>
>>>> 1) Linux can own (i.e have write access) completely one or more of video
>>>> ports along with corresponding resources (viz. overlay managers,
>>>> video planes) used by Linux in context of those video ports.
>>>> Even if Linux is owning
>>>> these video ports it can still share this video port with a remote core
>>>> which can own one or more video planes associated with this video port.
>>>>
>>>> 2) Linux owns one or more of the video planes with video port
>>>> (along with corresponding overlay manager) associated with these planes
>>>> being owned and controlled by a remote core. Linux still has read-only
>>>> access to the associated video port and overlay managers so that it can
>>>> parse the settings made by remote core.
>>>
>>> So, just to make sure we're on the same page. 1) means Linux drives the
>>> whole display engine, but can lend planes to the R5? How does that work,
>>> is Linux aware of the workload being there (plane size, format, etc) ?
>>>
>>
>> Well, there is no dynamic procedure being followed for lending. The
>> partitioning scheme is decided and known before hand, and the remote
>> core firmware updated and compiled accordingly, and similarly the
>> device-tree overlay for Linux is also updated with partitioning
>> information before bootup.
>>
>> What would happen here is that Linux will know before-hand this
>> partitioning information via device-tree properties and won't enumerate
>> the plane owned by RTOS, but it will enumerate the rest of the display
>> components and initialize the DSS, after which user can load the DSS
>> firmware on remote core and this firmware will only have control of
>> plane as it was compiled with that configuration.
> 
> Right. If the RTOS is in control of a single plane, how it is expected
> to deal with Linux shutting the CRTC down, or enforcing a configuration
> that isn't compatible with what the RTOS expects (like a plane with a
> higher zpos masking its plane), what is the mechanism to reconcile it?
> 

Just for the note, for this "RTOS control single plane" mode, we don't have a
firmware available to test (right now we are only supporting example for "RTOS
controlling the display mode" as shared here [1]) and hence this is not
validated but the idea was to keep dt-bindings generic enough to support them
in future and that's why I referred to it here.

Coming back to your questions, with the current scheme the Linux (tidss) would
be expected to make sure the CRTC being shared with RTOS is never shutdown and
the RTOS plane should never gets masked.

I think the IPC based scheme would have been mainly needed for the case where
you have a single entity controlling the display for e.g you have a single
display controller register space and a single IRQ but you have multiple
planes and say you want to divide these planes to different host processors.
In that case you want a single entity to act as a main entity and be in
control of DSS and rest of the processors communicate with the "main entity"
to request display resources and plane updates and main entity also programs
dss on their behalf.

But unlike above, TI DSS7 is designed to support static partitioning of
display resources among multiple hosts, where each host can program the
display hardware independently using separate register space and having a
separate irq and without requirement of any communication between the hosts.
Now as this feature is unique to TI DSS7 we want to support this feature in
tidss driver. The DSS resource partitioning feature is described in detail
here [2]

>>> And 2) would mean that the display engine is under the R5 control and
>>> Linux only gets to fill the plane and let the firmware know of what it
>>> wants?
>>>
>>
>> Here too the partitioning information is pre-decided and remote core
>> firmware and device-tree overlay for Linux updated accordingly. But in
>> this case as remote core firmware owns the display (minus the plane
>> owned by Linux) it is started and initialized during the bootloader
>> phase itself where it initializes the DSS and starts rendering using the
>> plane owned by it and Linux just latches to the DSS without
>> re-initializing it, with write access only to the plane that is owned by
>> Linux. You can refer [1] for more details on this.
>>
>>> If so, do we even need the tidss driver in the second case? We could
>>> just write a fwkms driver of some sorts that could be used by multiple
>>> implementations of the same "defer to firmware" logic.
>>>
>>
>> This feature of static partitioning of DSS resources is specific to DSS7
>> hardware (which is controlled by tidss driver) which supports dedicated
>> register space and interrupt line for each of the hosts [0], so that
>> multiple hosts can drive the display controller simultaneously as  per
>> the desired static partitioning of resources, and so I don't think a
>> separate driver is required here and tidss seems the right place to
>> support this, where using this device-tree approach different resource
>> partitioning schemas can be achieved as described here [1]. This was
>> also aligned with Tomi too where we discussed that tidss is the right
>> place to support this as we are simply leveraging the DSS hardware
>> capabilities of static partitioning here.
> 
> If the only thing tidss does in the "owned by RTOS" is forwarding KMS
> atomic states to the RTOS, then I'm still not sure why we need to
> involve tidss at all.
> 

I think maybe here is the point of misunderstanding. We are not forwarding
atomic states to RTOS here. Linux (tidss) is infact, accessing the display
register space assigned to it (common1 assigned to Linux, commmon0 assigned to
RTOS) and also writing to DSS plane registers for the plane assigned to it
(say VID assigned to Linux and VIDL assigned to RTOS).

> It's not just about interrupts, it's also about how your arbitrate
> between what Linux wants and what the RTOS wants. Like if the RTOS still
> wants to output something but Linux wants to disable it, how do you
> reconcile the two?
> 

The scheme involves static partitioning of display resource which are assigned
compile-time to RTOS and Linux. Here the RTOS firmware is compiled with
specific ownership/display resources as desired by user and this assignment
stays intact.

If there is a more complex use-case which requires dynamic
assignment/arbitration of resources then I agree those require some sort of
IPC scheme but this is not what we target with these series. This series is
simply to support static partitioning feature (separate register space,
separate irq, firewalling support etc) of TI DSS hardware across the multiple
hosts and there are use-cases too for which this scheme suffices.

> You have to have something that reconciles both, and typically for
> firmware-based setup this will be the firmware's job.
> 
> That's very similar to what the RaspberryPi did with fkms, and I believe
> that having a generic KMS-on-remoteproc driver when the firmware has
> control over the display is the path forward.
> 

The kms-on-remoteproc scheme is different and maybe more useful for those
processors not supporting this static partitioning hardware feature. On the
other hand, I believe there are still use-cases where this unique static
partitioning hardware feature of TI DSS will suffice without any requirement
of IPC. And it makes the firmware simpler (and the job of RTOS developer
easier too) as no IPC is required.

I am curious to understand Rpi DSS hardware and take a look at fkms and it's
firmware code though if it is public ?

>>>> For both the cases, the resources used in context of processing core
>>>> running Linux along with ownership information are exposed by user as
>>>> part of device-tree blob and driver uses an updated feature list tailored
>>>> for this shared mode accordingly. The driver also auto-populates
>>>> matching overlay managers and output types from shared video
>>>> port list provided in device-tree blob.
>>>> In dispc_feature struct remove const access specfier for output_type
>>>> array as it is required to be updated dynamically in run-time for shared
>>>> mode.
>>>
>>> I'm also not entirely sure that the device tree is the right path there.
>>> Surely the firmware capabilities will evolve over time, while the device
>>> tree won't. Is there some way to make it discoverable at probe time by
>>> the driver?
>>
>> I think the main highlight of the sharing feature is the hardware
>> capability where each host is provided separate irq and register space
>> to program display for their display context independently
> 
> Wait, what do you mean by display context here?
> 

By context I mean to what that specific host wants to display. For e.g. if
RTOS is owning a plane, then it can update it's framebuffer and update plane
registers independently.

[1]
https://software-dl.ti.com/processor-sdk-linux/esd/AM62PX/09_01_00_08/exports/docs/system/Demo_User_Guides/Display_Cluster_User_Guide.html
[2] https://www.ti.com/lit/zip/spruj52 ((Section 12.6.3.14 DISPC Resources
Sharing))

Regards
Devarsh

> Maxime

Re: [RFC PATCH 2/3] drm/tidss: Add support for display sharing

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 9329 bytes --]

Hi,

On Wed, Feb 14, 2024 at 09:17:12PM +0530, Devarsh Thakkar wrote:
> On 13/02/24 19:34, Maxime Ripard wrote:
> > On Thu, Feb 08, 2024 at 06:26:17PM +0530, Devarsh Thakkar wrote:
> >> On 26/01/24 17:45, Maxime Ripard wrote:
> >>> Hi,
> >>>
> >>> Thanks a lot for working on that.
> >>>
> >>> On Tue, Jan 16, 2024 at 07:11:41PM +0530, Devarsh Thakkar wrote:
> >>>> Display subsystem present in TI Keystone family of devices supports sharing
> >>>> of display between multiple hosts as it provides separate register space
> >>>> (common* region) for each host to programming display controller and also a
> >>>> unique interrupt line for each host.
> >>>>
> >>>> This adds support for display sharing, by allowing partitioning of
> >>>> resources either at video port level or at video plane level as
> >>>> described below :
> >>>>
> >>>> 1) Linux can own (i.e have write access) completely one or more of video
> >>>> ports along with corresponding resources (viz. overlay managers,
> >>>> video planes) used by Linux in context of those video ports.
> >>>> Even if Linux is owning
> >>>> these video ports it can still share this video port with a remote core
> >>>> which can own one or more video planes associated with this video port.
> >>>>
> >>>> 2) Linux owns one or more of the video planes with video port
> >>>> (along with corresponding overlay manager) associated with these planes
> >>>> being owned and controlled by a remote core. Linux still has read-only
> >>>> access to the associated video port and overlay managers so that it can
> >>>> parse the settings made by remote core.
> >>>
> >>> So, just to make sure we're on the same page. 1) means Linux drives the
> >>> whole display engine, but can lend planes to the R5? How does that work,
> >>> is Linux aware of the workload being there (plane size, format, etc) ?
> >>>
> >>
> >> Well, there is no dynamic procedure being followed for lending. The
> >> partitioning scheme is decided and known before hand, and the remote
> >> core firmware updated and compiled accordingly, and similarly the
> >> device-tree overlay for Linux is also updated with partitioning
> >> information before bootup.
> >>
> >> What would happen here is that Linux will know before-hand this
> >> partitioning information via device-tree properties and won't enumerate
> >> the plane owned by RTOS, but it will enumerate the rest of the display
> >> components and initialize the DSS, after which user can load the DSS
> >> firmware on remote core and this firmware will only have control of
> >> plane as it was compiled with that configuration.
> > 
> > Right. If the RTOS is in control of a single plane, how it is expected
> > to deal with Linux shutting the CRTC down, or enforcing a configuration
> > that isn't compatible with what the RTOS expects (like a plane with a
> > higher zpos masking its plane), what is the mechanism to reconcile it?
> > 
> 
> Just for the note, for this "RTOS control single plane" mode, we don't have a
> firmware available to test (right now we are only supporting example for "RTOS
> controlling the display mode" as shared here [1]) and hence this is not
> validated but the idea was to keep dt-bindings generic enough to support them
> in future and that's why I referred to it here.
> 
> Coming back to your questions, with the current scheme the Linux (tidss) would
> be expected to make sure the CRTC being shared with RTOS is never shutdown and
> the RTOS plane should never gets masked.

I'm probably missing something then here, but if the Linux side of
things is expected to keep the current configuration and keep it active
for it to work, what use-case would it be useful for?

> I think the IPC based scheme would have been mainly needed for the case where
> you have a single entity controlling the display for e.g you have a single
> display controller register space and a single IRQ but you have multiple
> planes and say you want to divide these planes to different host processors.

And with, I assume, different OS on those host processors? Otherwise why
would we need to handle some planes at the firmware level?

> In that case you want a single entity to act as a main entity and be in
> control of DSS and rest of the processors communicate with the "main entity"
> to request display resources and plane updates and main entity also programs
> dss on their behalf.
> 
> But unlike above, TI DSS7 is designed to support static partitioning of
> display resources among multiple hosts, where each host can program the
> display hardware independently using separate register space and having a
> separate irq and without requirement of any communication between the hosts.
> Now as this feature is unique to TI DSS7 we want to support this feature in
> tidss driver. The DSS resource partitioning feature is described in detail
> here [2]

So, if I understand this properly, and in KMS terms, DSS7 can assign the
planes, CRTCs or encoders to a given VM or CPU, and you can segment the
hardware that way. It looks like a good way to split encoders between
VMs, but going back to the discussion about one plane being handled by
the firmware, I don't really see how it can work with something else
than splitting away the whole pipeline and having a VM claiming a CRTC
and encoder, and another VM claiming another pipeline.

Like, if they share either a CRTC or encoder, we will still go back to
the discussion about arbitration about who has the final word if the two
have conflicting requirements, or if it changes something the other
probably has to know about it.

> >>> And 2) would mean that the display engine is under the R5 control and
> >>> Linux only gets to fill the plane and let the firmware know of what it
> >>> wants?
> >>>
> >>
> >> Here too the partitioning information is pre-decided and remote core
> >> firmware and device-tree overlay for Linux updated accordingly. But in
> >> this case as remote core firmware owns the display (minus the plane
> >> owned by Linux) it is started and initialized during the bootloader
> >> phase itself where it initializes the DSS and starts rendering using the
> >> plane owned by it and Linux just latches to the DSS without
> >> re-initializing it, with write access only to the plane that is owned by
> >> Linux. You can refer [1] for more details on this.
> >>
> >>> If so, do we even need the tidss driver in the second case? We could
> >>> just write a fwkms driver of some sorts that could be used by multiple
> >>> implementations of the same "defer to firmware" logic.
> >>>
> >>
> >> This feature of static partitioning of DSS resources is specific to DSS7
> >> hardware (which is controlled by tidss driver) which supports dedicated
> >> register space and interrupt line for each of the hosts [0], so that
> >> multiple hosts can drive the display controller simultaneously as  per
> >> the desired static partitioning of resources, and so I don't think a
> >> separate driver is required here and tidss seems the right place to
> >> support this, where using this device-tree approach different resource
> >> partitioning schemas can be achieved as described here [1]. This was
> >> also aligned with Tomi too where we discussed that tidss is the right
> >> place to support this as we are simply leveraging the DSS hardware
> >> capabilities of static partitioning here.
> > 
> > If the only thing tidss does in the "owned by RTOS" is forwarding KMS
> > atomic states to the RTOS, then I'm still not sure why we need to
> > involve tidss at all.
>
> I think maybe here is the point of misunderstanding. We are not forwarding
> atomic states to RTOS here. Linux (tidss) is infact, accessing the display
> register space assigned to it (common1 assigned to Linux, commmon0 assigned to
> RTOS) and also writing to DSS plane registers for the plane assigned to it
> (say VID assigned to Linux and VIDL assigned to RTOS).
> 
> > It's not just about interrupts, it's also about how your arbitrate
> > between what Linux wants and what the RTOS wants. Like if the RTOS still
> > wants to output something but Linux wants to disable it, how do you
> > reconcile the two?
> > 
> 
> The scheme involves static partitioning of display resource which are assigned
> compile-time to RTOS and Linux. Here the RTOS firmware is compiled with
> specific ownership/display resources as desired by user and this assignment
> stays intact.
> 
> If there is a more complex use-case which requires dynamic
> assignment/arbitration of resources then I agree those require some sort of
> IPC scheme but this is not what we target with these series. This series is
> simply to support static partitioning feature (separate register space,
> separate irq, firewalling support etc) of TI DSS hardware across the multiple
> hosts and there are use-cases too for which this scheme suffices.

I think you're right and we have a misunderstanding. My initial
assumption was that it was to prevent the Linux side of sides from
screwing up the output if it was to crash.

But it looks like it's not the main point of this series, so could you
share some use-cases you're trying to address?

Thanks!
Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [RFC PATCH 2/3] drm/tidss: Add support for display sharing

[Devarsh Thakkar]

Hi Maxime,

On 14/03/24 20:04, Maxime Ripard wrote:
> Hi,
> 
> On Wed, Feb 14, 2024 at 09:17:12PM +0530, Devarsh Thakkar wrote:
>> On 13/02/24 19:34, Maxime Ripard wrote:
>>> On Thu, Feb 08, 2024 at 06:26:17PM +0530, Devarsh Thakkar wrote:
>>>> On 26/01/24 17:45, Maxime Ripard wrote:
>>>>> Hi,
>>>>>
>>>>> Thanks a lot for working on that.
>>>>>
>>>>> On Tue, Jan 16, 2024 at 07:11:41PM +0530, Devarsh Thakkar wrote:
>>>>>> Display subsystem present in TI Keystone family of devices supports sharing
>>>>>> of display between multiple hosts as it provides separate register space
>>>>>> (common* region) for each host to programming display controller and also a
>>>>>> unique interrupt line for each host.
>>>>>>
>>>>>> This adds support for display sharing, by allowing partitioning of
>>>>>> resources either at video port level or at video plane level as
>>>>>> described below :
>>>>>>
>>>>>> 1) Linux can own (i.e have write access) completely one or more of video
>>>>>> ports along with corresponding resources (viz. overlay managers,
>>>>>> video planes) used by Linux in context of those video ports.
>>>>>> Even if Linux is owning
>>>>>> these video ports it can still share this video port with a remote core
>>>>>> which can own one or more video planes associated with this video port.
>>>>>>
>>>>>> 2) Linux owns one or more of the video planes with video port
>>>>>> (along with corresponding overlay manager) associated with these planes
>>>>>> being owned and controlled by a remote core. Linux still has read-only
>>>>>> access to the associated video port and overlay managers so that it can
>>>>>> parse the settings made by remote core.
>>>>>
>>>>> So, just to make sure we're on the same page. 1) means Linux drives the
>>>>> whole display engine, but can lend planes to the R5? How does that work,
>>>>> is Linux aware of the workload being there (plane size, format, etc) ?
>>>>>
>>>>
>>>> Well, there is no dynamic procedure being followed for lending. The
>>>> partitioning scheme is decided and known before hand, and the remote
>>>> core firmware updated and compiled accordingly, and similarly the
>>>> device-tree overlay for Linux is also updated with partitioning
>>>> information before bootup.
>>>>
>>>> What would happen here is that Linux will know before-hand this
>>>> partitioning information via device-tree properties and won't enumerate
>>>> the plane owned by RTOS, but it will enumerate the rest of the display
>>>> components and initialize the DSS, after which user can load the DSS
>>>> firmware on remote core and this firmware will only have control of
>>>> plane as it was compiled with that configuration.
>>>
>>> Right. If the RTOS is in control of a single plane, how it is expected
>>> to deal with Linux shutting the CRTC down, or enforcing a configuration
>>> that isn't compatible with what the RTOS expects (like a plane with a
>>> higher zpos masking its plane), what is the mechanism to reconcile it?
>>>
>>
>> Just for the note, for this "RTOS control single plane" mode, we don't have a
>> firmware available to test (right now we are only supporting example for "RTOS
>> controlling the display mode" as shared here [1]) and hence this is not
>> validated but the idea was to keep dt-bindings generic enough to support them
>> in future and that's why I referred to it here.
>>
>> Coming back to your questions, with the current scheme the Linux (tidss) would
>> be expected to make sure the CRTC being shared with RTOS is never shutdown and
>> the RTOS plane should never gets masked.
> 
> I'm probably missing something then here, but if the Linux side of
> things is expected to keep the current configuration and keep it active
> for it to work, what use-case would it be useful for?
> 

It's just one of the partitioning possibilities that I mentioned here, that
Linux is in control of DSS as a whole and the user want the other host (be it
RTOS or any other core) to control a single plane. For e.g it could be Linux
(with GPU rendering) displaying the graphics and RTOS overlaying a real time
clock or any other signs which need to be displayed in real-time.
But more than the use-case this is inspired by the fact that we want to be
flexible and support in the linux driver whatever partitioning scheme
possibilities are there which are supported in hardware and we let user decide
on the partitioning scheme.

>> I think the IPC based scheme would have been mainly needed for the case where
>> you have a single entity controlling the display for e.g you have a single
>> display controller register space and a single IRQ but you have multiple
>> planes and say you want to divide these planes to different host processors.
> 
> And with, I assume, different OS on those host processors? Otherwise why
> would we need to handle some planes at the firmware level?
> 
>> In that case you want a single entity to act as a main entity and be in
>> control of DSS and rest of the processors communicate with the "main entity"
>> to request display resources and plane updates and main entity also programs
>> dss on their behalf.
>>
>> But unlike above, TI DSS7 is designed to support static partitioning of
>> display resources among multiple hosts, where each host can program the
>> display hardware independently using separate register space and having a
>> separate irq and without requirement of any communication between the hosts.
>> Now as this feature is unique to TI DSS7 we want to support this feature in
>> tidss driver. The DSS resource partitioning feature is described in detail
>> here [2]
> 
> So, if I understand this properly, and in KMS terms, DSS7 can assign the
> planes, CRTCs or encoders to a given VM or CPU, and you can segment the
> hardware that way. It looks like a good way to split encoders between
> VMs, but going back to the discussion about one plane being handled by
> the firmware, I don't really see how it can work with something else
> than splitting away the whole pipeline and having a VM claiming a CRTC
> and encoder, and another VM claiming another pipeline.
> 
> Like, if they share either a CRTC or encoder, we will still go back to
> the discussion about arbitration about who has the final word if the two
> have conflicting requirements, or if it changes something the other
> probably has to know about it.

There should not be any conflicting requirements as this sharing scheme is a
static one i.e. it is pre-negotiated or decided by the user before Linux
kernel compilation or RTOS firmware compilation and resources are split
statically and the sharing scheme is communicated to Linux via device-tree and
RTOS side firmware configured and compiled accordingly, and this scheme stays
intact without any change after device boots up. So for e.g. if Linux is
assigned only one plane and RTOS is the DSS master controlling all other
entities then this scheme will stay intact as long as device is up.

Also there could be only a single DSS master (the one in control of global
common0 reg space, i.e. having access to global DSS registers and also in
control of dss clock and power domains) and whichever core is acting as DSS
master will be knowing which of the CRTC and encoders are shared with other
hosts so that it doesn't power them off given other cores may still be using it.

> 
>>>>> And 2) would mean that the display engine is under the R5 control and
>>>>> Linux only gets to fill the plane and let the firmware know of what it
>>>>> wants?
>>>>>
>>>>
>>>> Here too the partitioning information is pre-decided and remote core
>>>> firmware and device-tree overlay for Linux updated accordingly. But in
>>>> this case as remote core firmware owns the display (minus the plane
>>>> owned by Linux) it is started and initialized during the bootloader
>>>> phase itself where it initializes the DSS and starts rendering using the
>>>> plane owned by it and Linux just latches to the DSS without
>>>> re-initializing it, with write access only to the plane that is owned by
>>>> Linux. You can refer [1] for more details on this.
>>>>
>>>>> If so, do we even need the tidss driver in the second case? We could
>>>>> just write a fwkms driver of some sorts that could be used by multiple
>>>>> implementations of the same "defer to firmware" logic.
>>>>>
>>>>
>>>> This feature of static partitioning of DSS resources is specific to DSS7
>>>> hardware (which is controlled by tidss driver) which supports dedicated
>>>> register space and interrupt line for each of the hosts [0], so that
>>>> multiple hosts can drive the display controller simultaneously as  per
>>>> the desired static partitioning of resources, and so I don't think a
>>>> separate driver is required here and tidss seems the right place to
>>>> support this, where using this device-tree approach different resource
>>>> partitioning schemas can be achieved as described here [1]. This was
>>>> also aligned with Tomi too where we discussed that tidss is the right
>>>> place to support this as we are simply leveraging the DSS hardware
>>>> capabilities of static partitioning here.
>>>
>>> If the only thing tidss does in the "owned by RTOS" is forwarding KMS
>>> atomic states to the RTOS, then I'm still not sure why we need to
>>> involve tidss at all.
>>
>> I think maybe here is the point of misunderstanding. We are not forwarding
>> atomic states to RTOS here. Linux (tidss) is infact, accessing the display
>> register space assigned to it (common1 assigned to Linux, commmon0 assigned to
>> RTOS) and also writing to DSS plane registers for the plane assigned to it
>> (say VID assigned to Linux and VIDL assigned to RTOS).
>>
>>> It's not just about interrupts, it's also about how your arbitrate
>>> between what Linux wants and what the RTOS wants. Like if the RTOS still
>>> wants to output something but Linux wants to disable it, how do you
>>> reconcile the two?
>>>
>>
>> The scheme involves static partitioning of display resource which are assigned
>> compile-time to RTOS and Linux. Here the RTOS firmware is compiled with
>> specific ownership/display resources as desired by user and this assignment
>> stays intact.
>>
>> If there is a more complex use-case which requires dynamic
>> assignment/arbitration of resources then I agree those require some sort of
>> IPC scheme but this is not what we target with these series. This series is
>> simply to support static partitioning feature (separate register space,
>> separate irq, firewalling support etc) of TI DSS hardware across the multiple
>> hosts and there are use-cases too for which this scheme suffices.
> 
> I think you're right and we have a misunderstanding. My initial
> assumption was that it was to prevent the Linux side of sides from
> screwing up the output if it was to crash.
> 
> But it looks like it's not the main point of this series, so could you
> share some use-cases you're trying to address?
> 

The end use-case we have demonstrated right now with this series is a
proof-of-concept display cluster use-case where RTOS boots early on MCU core
(launched at bootloader stage) and initializes the display (using the global
common0 register space and irq) and starts displaying safety tell-tales on one
plane, and once Linux boots up on application processor,
Linux (using common1 register space and irq) controls the other plane with GPU
rendering using a QT based application. And yes, we also support the scenario
where Linux crashes but RTOS being the DSS master and in control of DSS power,
clock domain and global register space is not impacted by the crash.
This is demonstrated in this video [1] and steps to simulate the same are also
documented here [2].

[1]: https://www.youtube.com/watch?v=WIcds6HGeMI&t=884s
[2]:
https://software-dl.ti.com/processor-sdk-linux/esd/AM62PX/09_01_00_08/exports/docs/system/Demo_User_Guides/Display_Cluster_User_Guide.html

Regards
Devarsh

> Thanks!
> Maxime

Re: [RFC PATCH 2/3] drm/tidss: Add support for display sharing

[Javier Martinez Canillas]

Devarsh Thakkar <devarsht@ti.com> writes:

Hello Devarsh and Maxime,

> Hi Maxime,
>
> On 14/03/24 20:04, Maxime Ripard wrote:
>> Hi,
>> 
>> On Wed, Feb 14, 2024 at 09:17:12PM +0530, Devarsh Thakkar wrote:
>>> On 13/02/24 19:34, Maxime Ripard wrote:
>>>> On Thu, Feb 08, 2024 at 06:26:17PM +0530, Devarsh Thakkar wrote:
>>>>> On 26/01/24 17:45, Maxime Ripard wrote:
>>>>>> Hi,
>>>>>>
>>>>>> Thanks a lot for working on that.
>>>>>>
>>>>>> On Tue, Jan 16, 2024 at 07:11:41PM +0530, Devarsh Thakkar wrote:
>>>>>>> Display subsystem present in TI Keystone family of devices supports sharing
>>>>>>> of display between multiple hosts as it provides separate register space
>>>>>>> (common* region) for each host to programming display controller and also a
>>>>>>> unique interrupt line for each host.
>>>>>>>
>>>>>>> This adds support for display sharing, by allowing partitioning of
>>>>>>> resources either at video port level or at video plane level as
>>>>>>> described below :
>>>>>>>
>>>>>>> 1) Linux can own (i.e have write access) completely one or more of video
>>>>>>> ports along with corresponding resources (viz. overlay managers,
>>>>>>> video planes) used by Linux in context of those video ports.
>>>>>>> Even if Linux is owning
>>>>>>> these video ports it can still share this video port with a remote core
>>>>>>> which can own one or more video planes associated with this video port.
>>>>>>>
>>>>>>> 2) Linux owns one or more of the video planes with video port
>>>>>>> (along with corresponding overlay manager) associated with these planes
>>>>>>> being owned and controlled by a remote core. Linux still has read-only
>>>>>>> access to the associated video port and overlay managers so that it can
>>>>>>> parse the settings made by remote core.
>>>>>>
>>>>>> So, just to make sure we're on the same page. 1) means Linux drives the
>>>>>> whole display engine, but can lend planes to the R5? How does that work,
>>>>>> is Linux aware of the workload being there (plane size, format, etc) ?
>>>>>>
>>>>>
>>>>> Well, there is no dynamic procedure being followed for lending. The
>>>>> partitioning scheme is decided and known before hand, and the remote
>>>>> core firmware updated and compiled accordingly, and similarly the
>>>>> device-tree overlay for Linux is also updated with partitioning
>>>>> information before bootup.
>>>>>
>>>>> What would happen here is that Linux will know before-hand this
>>>>> partitioning information via device-tree properties and won't enumerate
>>>>> the plane owned by RTOS, but it will enumerate the rest of the display
>>>>> components and initialize the DSS, after which user can load the DSS
>>>>> firmware on remote core and this firmware will only have control of
>>>>> plane as it was compiled with that configuration.
>>>>
>>>> Right. If the RTOS is in control of a single plane, how it is expected
>>>> to deal with Linux shutting the CRTC down, or enforcing a configuration
>>>> that isn't compatible with what the RTOS expects (like a plane with a
>>>> higher zpos masking its plane), what is the mechanism to reconcile it?
>>>>
>>>
>>> Just for the note, for this "RTOS control single plane" mode, we don't have a
>>> firmware available to test (right now we are only supporting example for "RTOS
>>> controlling the display mode" as shared here [1]) and hence this is not
>>> validated but the idea was to keep dt-bindings generic enough to support them
>>> in future and that's why I referred to it here.
>>>


If I understand you correctly, for now the only real use case is when the
the RTOS owns / manages the complete display pipeline and Linux can only
own video planes.

The opposite is supported by the DSS hardware (thanks to its feature that
allows partitioning the register space and having multiple per-host IRQs) 
but it's not a real use case yet. The reason why this case is added to the
DT binding is as you said for flexiblity and make the design future-proof.

>>> separate irq
>>> Coming back to your questions, with the current scheme the Linux (tidss) would
>>> be expected to make sure the CRTC being shared with RTOS is never shutdown and
>>> the RTOS plane should never gets masked.
>> 
>> I'm probably missing something then here, but if the Linux side of
>> things is expected to keep the current configuration and keep it active
>> for it to work, what use-case would it be useful for?
>> 
>
> It's just one of the partitioning possibilities that I mentioned here, that
> Linux is in control of DSS as a whole and the user want the other host (be it
> RTOS or any other core) to control a single plane. For e.g it could be Linux
> (with GPU rendering) displaying the graphics and RTOS overlaying a real time
> clock or any other signs which need to be displayed in real-time.
> But more than the use-case this is inspired by the fact that we want to be
> flexible and support in the linux driver whatever partitioning scheme
> possibilities are there which are supported in hardware and we let user decide
> on the partitioning scheme.
>

A possible use case here could be if Linux is safer than the other host
owning a single plane, right? Then in that case the RTOS could fail but
the display pipeline won't be teared down.

That is, if your safety tell-tales would be driven by Linux and having
other OS dislay the GPU-rendered QT based application on another plane.

But as said, for now that's a theorethical use case since the one you
mentioned is the opposite.

[....]

>>>
>>>> It's not just about interrupts, it's also about how your arbitrate
>>>> between what Linux wants and what the RTOS wants. Like if the RTOS still
>>>> wants to output something but Linux wants to disable it, how do you
>>>> reconcile the two?
>>>>
>>>
>>> The scheme involves static partitioning of display resource which are assigned
>>> compile-time to RTOS and Linux. Here the RTOS firmware is compiled with
>>> specific ownership/display resources as desired by user and this assignment
>>> stays intact.
>>>
>>> If there is a more complex use-case which requires dynamic
>>> assignment/arbitration of resources then I agree those require some sort of
>>> IPC scheme but this is not what we target with these series. This series is
>>> simply to support static partitioning feature (separate register space,
>>> separate irq, firewalling support etc) of TI DSS hardware across the multiple
>>> hosts and there are use-cases too for which this scheme suffices.
>> 
>> I think you're right and we have a misunderstanding. My initial
>> assumption was that it was to prevent the Linux side of sides from
>> screwing up the output if it was to crash.
>> 
>> But it looks like it's not the main point of this series, so could you
>> share some use-cases you're trying to address?
>> 
>
> The end use-case we have demonstrated right now with this series is a
> proof-of-concept display cluster use-case where RTOS boots early on MCU core
> (launched at bootloader stage) and initializes the display (using the global
> common0 register space and irq) and starts displaying safety tell-tales on one
> plane, and once Linux boots up on application processor,
> Linux (using common1 register space and irq) controls the other plane with GPU
> rendering using a QT based application. And yes, we also support the scenario
> where Linux crashes but RTOS being the DSS master and in control of DSS power,
> clock domain and global register space is not impacted by the crash.

You mention 2 scenarios but are actually the same? Or did I misunderstand?

In both cases the RTOS own the display pipeline and Linux can just display
using a single plane.

That's why I think that agree with Maxime, that a fwkms could be a simpler
solution to your use case instead of adding all this complexity to the DSS
driver. Yes, I understand the HW supports all this flexibility but there's
no real use case yet (you mentioned that don't even have firmware for this
single plane owned by the RTOS in the R5F case).

The DT binding for a fwkms driver would be trivial, in fact maybe we might
even leverage simpledrm for this case and not require a new driver at all.

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [RFC PATCH 2/3] drm/tidss: Add support for display sharing

[Daniel Vetter]

On Wed, May 15, 2024 at 04:45:09PM +0200, Javier Martinez Canillas wrote:
> Devarsh Thakkar <devarsht@ti.com> writes:
> 
> Hello Devarsh and Maxime,
> 
> > Hi Maxime,
> >
> > On 14/03/24 20:04, Maxime Ripard wrote:
> >> Hi,
> >> 
> >> On Wed, Feb 14, 2024 at 09:17:12PM +0530, Devarsh Thakkar wrote:
> >>> On 13/02/24 19:34, Maxime Ripard wrote:
> >>>> On Thu, Feb 08, 2024 at 06:26:17PM +0530, Devarsh Thakkar wrote:
> >>>>> On 26/01/24 17:45, Maxime Ripard wrote:
> >>>>>> Hi,
> >>>>>>
> >>>>>> Thanks a lot for working on that.
> >>>>>>
> >>>>>> On Tue, Jan 16, 2024 at 07:11:41PM +0530, Devarsh Thakkar wrote:
> >>>>>>> Display subsystem present in TI Keystone family of devices supports sharing
> >>>>>>> of display between multiple hosts as it provides separate register space
> >>>>>>> (common* region) for each host to programming display controller and also a
> >>>>>>> unique interrupt line for each host.
> >>>>>>>
> >>>>>>> This adds support for display sharing, by allowing partitioning of
> >>>>>>> resources either at video port level or at video plane level as
> >>>>>>> described below :
> >>>>>>>
> >>>>>>> 1) Linux can own (i.e have write access) completely one or more of video
> >>>>>>> ports along with corresponding resources (viz. overlay managers,
> >>>>>>> video planes) used by Linux in context of those video ports.
> >>>>>>> Even if Linux is owning
> >>>>>>> these video ports it can still share this video port with a remote core
> >>>>>>> which can own one or more video planes associated with this video port.
> >>>>>>>
> >>>>>>> 2) Linux owns one or more of the video planes with video port
> >>>>>>> (along with corresponding overlay manager) associated with these planes
> >>>>>>> being owned and controlled by a remote core. Linux still has read-only
> >>>>>>> access to the associated video port and overlay managers so that it can
> >>>>>>> parse the settings made by remote core.
> >>>>>>
> >>>>>> So, just to make sure we're on the same page. 1) means Linux drives the
> >>>>>> whole display engine, but can lend planes to the R5? How does that work,
> >>>>>> is Linux aware of the workload being there (plane size, format, etc) ?
> >>>>>>
> >>>>>
> >>>>> Well, there is no dynamic procedure being followed for lending. The
> >>>>> partitioning scheme is decided and known before hand, and the remote
> >>>>> core firmware updated and compiled accordingly, and similarly the
> >>>>> device-tree overlay for Linux is also updated with partitioning
> >>>>> information before bootup.
> >>>>>
> >>>>> What would happen here is that Linux will know before-hand this
> >>>>> partitioning information via device-tree properties and won't enumerate
> >>>>> the plane owned by RTOS, but it will enumerate the rest of the display
> >>>>> components and initialize the DSS, after which user can load the DSS
> >>>>> firmware on remote core and this firmware will only have control of
> >>>>> plane as it was compiled with that configuration.
> >>>>
> >>>> Right. If the RTOS is in control of a single plane, how it is expected
> >>>> to deal with Linux shutting the CRTC down, or enforcing a configuration
> >>>> that isn't compatible with what the RTOS expects (like a plane with a
> >>>> higher zpos masking its plane), what is the mechanism to reconcile it?
> >>>>
> >>>
> >>> Just for the note, for this "RTOS control single plane" mode, we don't have a
> >>> firmware available to test (right now we are only supporting example for "RTOS
> >>> controlling the display mode" as shared here [1]) and hence this is not
> >>> validated but the idea was to keep dt-bindings generic enough to support them
> >>> in future and that's why I referred to it here.
> >>>
> 
> 
> If I understand you correctly, for now the only real use case is when the
> the RTOS owns / manages the complete display pipeline and Linux can only
> own video planes.
> 
> The opposite is supported by the DSS hardware (thanks to its feature that
> allows partitioning the register space and having multiple per-host IRQs) 
> but it's not a real use case yet. The reason why this case is added to the
> DT binding is as you said for flexiblity and make the design future-proof.
> 
> >>> separate irq
> >>> Coming back to your questions, with the current scheme the Linux (tidss) would
> >>> be expected to make sure the CRTC being shared with RTOS is never shutdown and
> >>> the RTOS plane should never gets masked.
> >> 
> >> I'm probably missing something then here, but if the Linux side of
> >> things is expected to keep the current configuration and keep it active
> >> for it to work, what use-case would it be useful for?
> >> 
> >
> > It's just one of the partitioning possibilities that I mentioned here, that
> > Linux is in control of DSS as a whole and the user want the other host (be it
> > RTOS or any other core) to control a single plane. For e.g it could be Linux
> > (with GPU rendering) displaying the graphics and RTOS overlaying a real time
> > clock or any other signs which need to be displayed in real-time.
> > But more than the use-case this is inspired by the fact that we want to be
> > flexible and support in the linux driver whatever partitioning scheme
> > possibilities are there which are supported in hardware and we let user decide
> > on the partitioning scheme.
> >
> 
> A possible use case here could be if Linux is safer than the other host
> owning a single plane, right? Then in that case the RTOS could fail but
> the display pipeline won't be teared down.
> 
> That is, if your safety tell-tales would be driven by Linux and having
> other OS dislay the GPU-rendered QT based application on another plane.
> 
> But as said, for now that's a theorethical use case since the one you
> mentioned is the opposite.
> 
> [....]
> 
> >>>
> >>>> It's not just about interrupts, it's also about how your arbitrate
> >>>> between what Linux wants and what the RTOS wants. Like if the RTOS still
> >>>> wants to output something but Linux wants to disable it, how do you
> >>>> reconcile the two?
> >>>>
> >>>
> >>> The scheme involves static partitioning of display resource which are assigned
> >>> compile-time to RTOS and Linux. Here the RTOS firmware is compiled with
> >>> specific ownership/display resources as desired by user and this assignment
> >>> stays intact.
> >>>
> >>> If there is a more complex use-case which requires dynamic
> >>> assignment/arbitration of resources then I agree those require some sort of
> >>> IPC scheme but this is not what we target with these series. This series is
> >>> simply to support static partitioning feature (separate register space,
> >>> separate irq, firewalling support etc) of TI DSS hardware across the multiple
> >>> hosts and there are use-cases too for which this scheme suffices.
> >> 
> >> I think you're right and we have a misunderstanding. My initial
> >> assumption was that it was to prevent the Linux side of sides from
> >> screwing up the output if it was to crash.
> >> 
> >> But it looks like it's not the main point of this series, so could you
> >> share some use-cases you're trying to address?
> >> 
> >
> > The end use-case we have demonstrated right now with this series is a
> > proof-of-concept display cluster use-case where RTOS boots early on MCU core
> > (launched at bootloader stage) and initializes the display (using the global
> > common0 register space and irq) and starts displaying safety tell-tales on one
> > plane, and once Linux boots up on application processor,
> > Linux (using common1 register space and irq) controls the other plane with GPU
> > rendering using a QT based application. And yes, we also support the scenario
> > where Linux crashes but RTOS being the DSS master and in control of DSS power,
> > clock domain and global register space is not impacted by the crash.
> 
> You mention 2 scenarios but are actually the same? Or did I misunderstand?
> 
> In both cases the RTOS own the display pipeline and Linux can just display
> using a single plane.
> 
> That's why I think that agree with Maxime, that a fwkms could be a simpler
> solution to your use case instead of adding all this complexity to the DSS
> driver. Yes, I understand the HW supports all this flexibility but there's
> no real use case yet (you mentioned that don't even have firmware for this
> single plane owned by the RTOS in the R5F case).
> 
> The DT binding for a fwkms driver would be trivial, in fact maybe we might
> even leverage simpledrm for this case and not require a new driver at all.

I guess you can still do things like pageflipping and maybe use some of
the color/blending hardware? Maybe even have more than one plane
available? fwkms/simpledrm conceptually cannot really support pageflipping
even, so that's a much, much reduced feature set.

That all aside I do think we should limit the support to just the first
case, where linux gets a few pieces assigned to it and is not the DSS
master. From what I'm understanding you could assign entire crtc with
planes and everything to linux, so this shouldn't really constraint
real-world usage?

At least until there's support in firmware for this it's all way too
theoretical, and I agree with Maxime and Javier that there's some serious
design questions about how this kind of static leasing should work with
drm sitting on top.
-Sima
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch