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
>
