Received-SPF: pass (google.com: domain of linux-kernel+bounces-29404-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Wed, 17 Jan 2024 14:13:42 -0600
From: Rob Herring <robh@kernel.org>
To: Devarsh Thakkar <devarsht@ti.com>
Cc: jyri.sarha@iki.fi, tomi.valkeinen@ideasonboard.com, airlied@gmail.com,
	daniel@ffwll.ch, maarten.lankhorst@linux.intel.com,
	mripard@kernel.org, tzimmermann@suse.de,
	krzysztof.kozlowski+dt@linaro.org, conor+dt@kernel.org,
	dri-devel@lists.freedesktop.org, devicetree@vger.kernel.org,
	linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
	nm@ti.com, vigneshr@ti.com, kristo@kernel.org, praneeth@ti.com,
	a-bhatia1@ti.com, j-luthra@ti.com
Subject: Re: [RFC PATCH 1/3] dt-bindings: display: ti,am65x-dss: Add support
 for display sharing mode
Message-ID: <20240117201342.GA3041972-robh@kernel.org>
References: <20240116134142.2092483-1-devarsht@ti.com>
 <20240116134142.2092483-2-devarsht@ti.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240116134142.2092483-2-devarsht@ti.com>

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