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@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-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
>