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@xxxxxx> 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@xxxxxx>
> >> ---
> >>  .../bindings/display/ti/ti,am65x-dss.yaml     | 82 +++++++++++++++++++
> >>  1 file changed, 82 insertions(+)
> >>
> >> diff --git a/Documentation/devicetree/bindings/display/ti/ti,am65x-dssyaml 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