mbox series

[v7,0/6] Exynos: Simple QoS for exynos-bus using interconnect

Message ID 20201030125149.8227-1-s.nawrocki@samsung.com (mailing list archive)
Headers show
Series Exynos: Simple QoS for exynos-bus using interconnect | expand

Message

Sylwester Nawrocki Oct. 30, 2020, 12:51 p.m. UTC
This patchset adds interconnect API support for the Exynos SoC "samsung,
exynos-bus" compatible devices, which already have their corresponding
exynos-bus driver in the devfreq subsystem.  Complementing the devfreq
driver with an interconnect functionality allows to ensure the QoS
requirements of devices accessing the system memory (e.g. video processing
devices) are fulfilled and aallows to avoid issues like the one discussed
in thread [1].

This patch series adds implementation of the interconnect provider per each
"samsung,exynos-bus" compatible DT node, with one interconnect node per
provider.  The interconnect code which was previously added as a part of
the devfreq driver has been converted to a separate platform driver.
In the devfreq a corresponding virtual child platform device is registered.
Integration of devfreq and interconnect frameworks is achieved through
the PM QoS API.

A sample interconnect consumer for exynos-mixer is added in patches 5/6,
6/6, it is currently added only for exynos4412 and allows to address the
mixer DMA underrun error issues [1].

Changes since v6:
 - the interconnect consumer DT bindings are now used to describe dependencies
   of the interconnects (samsung,exynos-bus nodes),
 - bus-width property replaced with samsung,data-clk-ratio,
 - adaptation to recent changes in the interconnect code
   (of_icc_get_from_provider(), icc_node_add()).

The series has been tested on Odroid U3 board. It is based on v5.10-rc1.

--
Regards,
Sylwester


Changes since v5:
 - addition of "bus-width: DT property, which specifies data width
   of the interconnect bus (patches 1...2/6),
 - addition of synchronization of the interconnect bandwidth setting
   with VSYNC (patch 6/6).

Changes since v3 [4] (v4 skipped to align with patchset [1]), detailed
changes are listed in each patch:
 - conversion to a separate interconnect (platform) driver,
 - an update of the DT binding documenting new optional properties:
   #interconnect-cells, samsung,interconnect-parent in "samsung,exynos-bus"
   nodes,
 - new DT properties added to the SoC, rather than to the board specific
   files.

Changes since v2 [5]:
 - Use icc_std_aggregate().
 - Implement a different modification of apply_constraints() in
   drivers/interconnect/core.c (patch 03).
 - Use 'exynos,interconnect-parent-node' in the DT instead of
   'devfreq'/'parent', depending on the bus.
 - Rebase on DT patches that deprecate the 'devfreq' DT property.
 - Improve error handling, including freeing generated IDs on failure.
 - Remove exynos_bus_icc_connect() and add exynos_bus_icc_get_parent().

Changes since v1 [6]:
 - Rebase on coupled regulators patches.
 - Use dev_pm_qos_*() API instead of overriding frequency in
   exynos_bus_target().
 - Use IDR for node ID allocation.
 - Reverse order of multiplication and division in
   mixer_set_memory_bandwidth() (patch 07) to avoid integer overflow.


References:
[1] https://patchwork.kernel.org/patch/10861757/ (original issue)
[2] https://www.spinics.net/lists/linux-samsung-soc/msg70014.html
[3] https://www.spinics.net/lists/arm-kernel/msg810722.html
[4] https://lore.kernel.org/linux-pm/20191220115653.6487-1-a.swigon@samsung.com
[5] https://patchwork.kernel.org/cover/11054417/ (v1 of this RFC)
[6] https://patchwork.kernel.org/cover/11152595/ (v2 of this RFC)


Artur Świgoń (1):
  ARM: dts: exynos: Add interconnects to Exynos4412 mixer

Sylwester Nawrocki (5):
  dt-bindings: devfreq: Add documentation for the interconnect
    properties
  interconnect: Add generic interconnect driver for Exynos SoCs
  PM / devfreq: exynos-bus: Add registration of interconnect child
    device
  ARM: dts: exynos: Add interconnect properties to Exynos4412 bus nodes
  drm: exynos: mixer: Add interconnect support

 .../devicetree/bindings/devfreq/exynos-bus.txt     |  68 ++++++-
 arch/arm/boot/dts/exynos4412.dtsi                  |   7 +
 drivers/devfreq/exynos-bus.c                       |  17 ++
 drivers/gpu/drm/exynos/exynos_mixer.c              | 146 ++++++++++++++-
 drivers/interconnect/Kconfig                       |   1 +
 drivers/interconnect/Makefile                      |   1 +
 drivers/interconnect/exynos/Kconfig                |   6 +
 drivers/interconnect/exynos/Makefile               |   4 +
 drivers/interconnect/exynos/exynos.c               | 198 +++++++++++++++++++++
 9 files changed, 438 insertions(+), 10 deletions(-)
 create mode 100644 drivers/interconnect/exynos/Kconfig
 create mode 100644 drivers/interconnect/exynos/Makefile
 create mode 100644 drivers/interconnect/exynos/exynos.c

--
2.7.4

Comments

Chanwoo Choi Nov. 3, 2020, 7:54 a.m. UTC | #1
Hi Sylwester,

When I tested this patchset on Odroid-U3,
After setting 0 bps by interconnect[1][2],
the frequency of devfreq devs sustain the high frequency
according to the pm qos request.

So, I try to find the cause of this situation.
In result, it seems that interconnect exynos driver
updates the pm qos request to devfreq device
during the kernel booting. Do you know why the exynos
interconnect driver request the pm qos during probe
without the mixer request?

PS. The passive governor has a bug related to PM_QOS interface.
So, I posted the patch[4].


[1] interconnect_graph
root@localhost:~# cat /sys/kernel/debug/interconnect/interconnect_graph 
digraph {
        rankdir = LR
        node [shape = record]
        subgraph cluster_1 {
                label = "soc:bus_dmc"
                "2:bus_dmc" [label="2:bus_dmc
                        |avg_bw=0kBps
                        |peak_bw=0kBps"]
        }
        subgraph cluster_2 {
                label = "soc:bus_leftbus"
                "3:bus_leftbus" [label="3:bus_leftbus
                        |avg_bw=0kBps
                        |peak_bw=0kBps"]
        }
        subgraph cluster_3 {
                label = "soc:bus_display"
                "4:bus_display" [label="4:bus_display
                        |avg_bw=0kBps
                        |peak_bw=0kBps"]
        }
        "3:bus_leftbus" -> "2:bus_dmc"
        "4:bus_display" -> "3:bus_leftbus"


[2] interconnect_summary
root@localhost:~# cat /sys/kernel/debug/interconnect/interconnect_summary 
 node                                  tag          avg         peak
--------------------------------------------------------------------
bus_dmc                                               0            0
  12c10000.mixer                         0            0            0
bus_leftbus                                           0            0
  12c10000.mixer                         0            0            0
bus_display                                           0            0
  12c10000.mixer                         0            0            0


[3] devfreq_summary
root@localhost:~# cat /sys/kernel/debug/devfreq/devfreq_summary 
dev                            parent_dev                     governor        timer      polling_ms  cur_freq_Hz  min_freq_Hz  max_freq_Hz
------------------------------ ------------------------------ --------------- ---------- ---------- ------------ ------------ ------------
soc:bus_dmc                    null                           simple_ondemand deferrable         50    400000000    400000000    400000000
soc:bus_acp                    soc:bus_dmc                    passive         null                0    267000000    100000000    267000000
soc:bus_c2c                    soc:bus_dmc                    passive         null                0    400000000    100000000    400000000
soc:bus_leftbus                null                           simple_ondemand deferrable         50    200000000    200000000    200000000
soc:bus_rightbus               soc:bus_leftbus                passive         null                0    200000000    100000000    200000000
soc:bus_display                soc:bus_leftbus                passive         null                0    200000000    200000000    200000000
soc:bus_fsys                   soc:bus_leftbus                passive         null                0    134000000    100000000    134000000
soc:bus_peri                   soc:bus_leftbus                passive         null                0    100000000     50000000    100000000
soc:bus_mfc                    soc:bus_leftbus                passive         null                0    200000000    100000000    200000000


[4] PM / devfreq: passive: Update frequency when start governor
https://patchwork.kernel.org/project/linux-pm/patch/20201103070646.18687-1-cw00.choi@samsung.com/


On 10/30/20 9:51 PM, Sylwester Nawrocki wrote:
> 
> This patchset adds interconnect API support for the Exynos SoC "samsung,
> exynos-bus" compatible devices, which already have their corresponding
> exynos-bus driver in the devfreq subsystem.  Complementing the devfreq
> driver with an interconnect functionality allows to ensure the QoS
> requirements of devices accessing the system memory (e.g. video processing
> devices) are fulfilled and aallows to avoid issues like the one discussed
> in thread [1].
> 
> This patch series adds implementation of the interconnect provider per each
> "samsung,exynos-bus" compatible DT node, with one interconnect node per
> provider.  The interconnect code which was previously added as a part of
> the devfreq driver has been converted to a separate platform driver.
> In the devfreq a corresponding virtual child platform device is registered.
> Integration of devfreq and interconnect frameworks is achieved through
> the PM QoS API.
> 
> A sample interconnect consumer for exynos-mixer is added in patches 5/6,
> 6/6, it is currently added only for exynos4412 and allows to address the
> mixer DMA underrun error issues [1].
> 
> Changes since v6:
>  - the interconnect consumer DT bindings are now used to describe dependencies
>    of the interconnects (samsung,exynos-bus nodes),
>  - bus-width property replaced with samsung,data-clk-ratio,
>  - adaptation to recent changes in the interconnect code
>    (of_icc_get_from_provider(), icc_node_add()).
> 
> The series has been tested on Odroid U3 board. It is based on v5.10-rc1.
> 
> --
> Regards,
> Sylwester
> 
> 
> Changes since v5:
>  - addition of "bus-width: DT property, which specifies data width
>    of the interconnect bus (patches 1...2/6),
>  - addition of synchronization of the interconnect bandwidth setting
>    with VSYNC (patch 6/6).
> 
> Changes since v3 [4] (v4 skipped to align with patchset [1]), detailed
> changes are listed in each patch:
>  - conversion to a separate interconnect (platform) driver,
>  - an update of the DT binding documenting new optional properties:
>    #interconnect-cells, samsung,interconnect-parent in "samsung,exynos-bus"
>    nodes,
>  - new DT properties added to the SoC, rather than to the board specific
>    files.
> 
> Changes since v2 [5]:
>  - Use icc_std_aggregate().
>  - Implement a different modification of apply_constraints() in
>    drivers/interconnect/core.c (patch 03).
>  - Use 'exynos,interconnect-parent-node' in the DT instead of
>    'devfreq'/'parent', depending on the bus.
>  - Rebase on DT patches that deprecate the 'devfreq' DT property.
>  - Improve error handling, including freeing generated IDs on failure.
>  - Remove exynos_bus_icc_connect() and add exynos_bus_icc_get_parent().
> 
> Changes since v1 [6]:
>  - Rebase on coupled regulators patches.
>  - Use dev_pm_qos_*() API instead of overriding frequency in
>    exynos_bus_target().
>  - Use IDR for node ID allocation.
>  - Reverse order of multiplication and division in
>    mixer_set_memory_bandwidth() (patch 07) to avoid integer overflow.
> 
> 
> References:
> [1] https://patchwork.kernel.org/patch/10861757/ (original issue)
> [2] https://protect2.fireeye.com/v1/url?k=383efc40-67a5c559-383f770f-000babff3793-a505fcd0b7477e5e&q=1&e=ad8ffb9f-f90b-49a7-a3df-2ab066a8c4ee&u=https%3A%2F%2Fwww.spinics.net%2Flists%2Flinux-samsung-soc%2Fmsg70014.html
> [3] https://protect2.fireeye.com/v1/url?k=13f0c488-4c6bfd91-13f14fc7-000babff3793-98a59bf1c5c6f1fb&q=1&e=ad8ffb9f-f90b-49a7-a3df-2ab066a8c4ee&u=https%3A%2F%2Fwww.spinics.net%2Flists%2Farm-kernel%2Fmsg810722.html
> [4] https://lore.kernel.org/linux-pm/20191220115653.6487-1-a.swigon@samsung.com
> [5] https://patchwork.kernel.org/cover/11054417/ (v1 of this RFC)
> [6] https://patchwork.kernel.org/cover/11152595/ (v2 of this RFC)
> 
> 
> Artur Świgoń (1):
>   ARM: dts: exynos: Add interconnects to Exynos4412 mixer
> 
> Sylwester Nawrocki (5):
>   dt-bindings: devfreq: Add documentation for the interconnect
>     properties
>   interconnect: Add generic interconnect driver for Exynos SoCs
>   PM / devfreq: exynos-bus: Add registration of interconnect child
>     device
>   ARM: dts: exynos: Add interconnect properties to Exynos4412 bus nodes
>   drm: exynos: mixer: Add interconnect support
> 
>  .../devicetree/bindings/devfreq/exynos-bus.txt     |  68 ++++++-
>  arch/arm/boot/dts/exynos4412.dtsi                  |   7 +
>  drivers/devfreq/exynos-bus.c                       |  17 ++
>  drivers/gpu/drm/exynos/exynos_mixer.c              | 146 ++++++++++++++-
>  drivers/interconnect/Kconfig                       |   1 +
>  drivers/interconnect/Makefile                      |   1 +
>  drivers/interconnect/exynos/Kconfig                |   6 +
>  drivers/interconnect/exynos/Makefile               |   4 +
>  drivers/interconnect/exynos/exynos.c               | 198 +++++++++++++++++++++
>  9 files changed, 438 insertions(+), 10 deletions(-)
>  create mode 100644 drivers/interconnect/exynos/Kconfig
>  create mode 100644 drivers/interconnect/exynos/Makefile
>  create mode 100644 drivers/interconnect/exynos/exynos.c
> 
> --
> 2.7.4
> 
> 
>
Georgi Djakov Nov. 3, 2020, 8:29 a.m. UTC | #2
Hi Chanwoo and Sylwester,

On 11/3/20 09:54, Chanwoo Choi wrote:
> Hi Sylwester,
> 
> When I tested this patchset on Odroid-U3,
> After setting 0 bps by interconnect[1][2],
> the frequency of devfreq devs sustain the high frequency
> according to the pm qos request.
> 
> So, I try to find the cause of this situation.
> In result, it seems that interconnect exynos driver
> updates the pm qos request to devfreq device
> during the kernel booting. Do you know why the exynos
> interconnect driver request the pm qos during probe
> without the mixer request?

That's probably because of the sync_state support, that was introduced
recently. The icc_sync_state callback needs to be added to the driver
(i just left a comment on that patch), and then check again if it works.

The idea of the sync_state is that there could be multiple users of a
path and we must wait for all consumers to tell their bandwidth needs.
Otherwise the first consumer may lower the bandwidth or disable a path
needed for another consumer (driver), which has not probed yet. So we
maintain a floor bandwidth until everyone has probed. By default the floor
bandwidth is INT_MAX, but can be overridden by implementing the get_bw()
callback.

Thanks,
Georgi

> 
> PS. The passive governor has a bug related to PM_QOS interface.
> So, I posted the patch[4].
> 
> 
> [1] interconnect_graph
> root@localhost:~# cat /sys/kernel/debug/interconnect/interconnect_graph 
> digraph {
>         rankdir = LR
>         node [shape = record]
>         subgraph cluster_1 {
>                 label = "soc:bus_dmc"
>                 "2:bus_dmc" [label="2:bus_dmc
>                         |avg_bw=0kBps
>                         |peak_bw=0kBps"]
>         }
>         subgraph cluster_2 {
>                 label = "soc:bus_leftbus"
>                 "3:bus_leftbus" [label="3:bus_leftbus
>                         |avg_bw=0kBps
>                         |peak_bw=0kBps"]
>         }
>         subgraph cluster_3 {
>                 label = "soc:bus_display"
>                 "4:bus_display" [label="4:bus_display
>                         |avg_bw=0kBps
>                         |peak_bw=0kBps"]
>         }
>         "3:bus_leftbus" -> "2:bus_dmc"
>         "4:bus_display" -> "3:bus_leftbus"
> 
> 
> [2] interconnect_summary
> root@localhost:~# cat /sys/kernel/debug/interconnect/interconnect_summary 
>  node                                  tag          avg         peak
> --------------------------------------------------------------------
> bus_dmc                                               0            0
>   12c10000.mixer                         0            0            0
> bus_leftbus                                           0            0
>   12c10000.mixer                         0            0            0
> bus_display                                           0            0
>   12c10000.mixer                         0            0            0
> 
> 
> [3] devfreq_summary
> root@localhost:~# cat /sys/kernel/debug/devfreq/devfreq_summary 
> dev                            parent_dev                     governor        timer      polling_ms  cur_freq_Hz  min_freq_Hz  max_freq_Hz
> ------------------------------ ------------------------------ --------------- ---------- ---------- ------------ ------------ ------------
> soc:bus_dmc                    null                           simple_ondemand deferrable         50    400000000    400000000    400000000
> soc:bus_acp                    soc:bus_dmc                    passive         null                0    267000000    100000000    267000000
> soc:bus_c2c                    soc:bus_dmc                    passive         null                0    400000000    100000000    400000000
> soc:bus_leftbus                null                           simple_ondemand deferrable         50    200000000    200000000    200000000
> soc:bus_rightbus               soc:bus_leftbus                passive         null                0    200000000    100000000    200000000
> soc:bus_display                soc:bus_leftbus                passive         null                0    200000000    200000000    200000000
> soc:bus_fsys                   soc:bus_leftbus                passive         null                0    134000000    100000000    134000000
> soc:bus_peri                   soc:bus_leftbus                passive         null                0    100000000     50000000    100000000
> soc:bus_mfc                    soc:bus_leftbus                passive         null                0    200000000    100000000    200000000
> 
> 
> [4] PM / devfreq: passive: Update frequency when start governor
> https://patchwork.kernel.org/project/linux-pm/patch/20201103070646.18687-1-cw00.choi@samsung.com/
> 
>
Chanwoo Choi Nov. 3, 2020, 8:53 a.m. UTC | #3
Hi Georgi,

On 11/3/20 5:29 PM, Georgi Djakov wrote:
> Hi Chanwoo and Sylwester,
> 
> On 11/3/20 09:54, Chanwoo Choi wrote:
>> Hi Sylwester,
>>
>> When I tested this patchset on Odroid-U3,
>> After setting 0 bps by interconnect[1][2],
>> the frequency of devfreq devs sustain the high frequency
>> according to the pm qos request.
>>
>> So, I try to find the cause of this situation.
>> In result, it seems that interconnect exynos driver
>> updates the pm qos request to devfreq device
>> during the kernel booting. Do you know why the exynos
>> interconnect driver request the pm qos during probe
>> without the mixer request?
> 
> That's probably because of the sync_state support, that was introduced
> recently. The icc_sync_state callback needs to be added to the driver
> (i just left a comment on that patch), and then check again if it works.
> 
> The idea of the sync_state is that there could be multiple users of a
> path and we must wait for all consumers to tell their bandwidth needs.
> Otherwise the first consumer may lower the bandwidth or disable a path
> needed for another consumer (driver), which has not probed yet. So we
> maintain a floor bandwidth until everyone has probed. By default the floor
> bandwidth is INT_MAX, but can be overridden by implementing the get_bw()
> callback.

Thanks for guide. I tested it with your comment of patch2.
It is well working without problem as I mentioned previously.

I caught the reset operation of PM QoS requested from interconnect
on kernel log. In result, after completed the kernel booting,
there is no pm qos request if hdmi cable is not connected.

[Test Result]
1. Set 622080 Bps with HDMI cable

root@localhost:~# cat /sys/kernel/debug/devfreq/devfreq_summary;
dev                            parent_dev                     governor        timer      polling_ms  cur_freq_Hz  min_freq_Hz  max_freq_Hz
------------------------------ ------------------------------ --------------- ---------- ---------- ------------ ------------ ------------
soc:bus_dmc                    null                           simple_ondemand deferrable         50    400000000    155520000    400000000
soc:bus_acp                    soc:bus_dmc                    passive         null                0    267000000    100000000    267000000
soc:bus_c2c                    soc:bus_dmc                    passive         null                0    400000000    100000000    400000000
soc:bus_leftbus                null                           simple_ondemand deferrable         50    200000000    100000000    200000000
soc:bus_rightbus               soc:bus_leftbus                passive         null                0    200000000    100000000    200000000
soc:bus_display                soc:bus_leftbus                passive         null                0    200000000    160000000    200000000
soc:bus_fsys                   soc:bus_leftbus                passive         null                0    134000000    100000000    134000000
soc:bus_peri                   soc:bus_leftbus                passive         null                0    100000000     50000000    100000000
soc:bus_mfc                    soc:bus_leftbus                passive         null                0    200000000    100000000    200000000
root@localhost:~# cat /sys/kernel/debug/interconnect/interconnect_graph;
digraph {
        rankdir = LR
        node [shape = record]
        subgraph cluster_1 {
                label = "soc:bus_dmc"
                "2:bus_dmc" [label="2:bus_dmc
                        |avg_bw=622080kBps
                        |peak_bw=622080kBps"]
        }
        subgraph cluster_2 {
                label = "soc:bus_leftbus"
                "3:bus_leftbus" [label="3:bus_leftbus
                        |avg_bw=622080kBps
                        |peak_bw=622080kBps"]
        }
        subgraph cluster_3 {
                label = "soc:bus_display"
                "4:bus_display" [label="4:bus_display
                        |avg_bw=622080kBps
                        |peak_bw=622080kBps"]
        }
        "3:bus_leftbus" -> "2:bus_dmc"
        "4:bus_display" -> "3:bus_leftbus"
}root@localhost:~# cat /sys/kernel/debug/interconnect/interconnect_summary;
 node                                  tag          avg         peak
--------------------------------------------------------------------
bus_dmc                                          622080       622080
  12c10000.mixer                         0       622080       622080
bus_leftbus                                      622080       622080
  12c10000.mixer                         0       622080       622080
bus_display                                      622080       622080
  12c10000.mixer                         0       622080       622080



2. Set 0Bps without HDMI cable
root@localhost:~# cat /sys/kernel/debug/devfreq/devfreq_summary;
dev                            parent_dev                     governor        timer      polling_ms  cur_freq_Hz  min_freq_Hz  max_freq_Hz
------------------------------ ------------------------------ --------------- ---------- ---------- ------------ ------------ ------------
soc:bus_dmc                    null                           simple_ondemand deferrable         50    100000000    100000000    400000000
soc:bus_acp                    soc:bus_dmc                    passive         null                0    100000000    100000000    267000000
soc:bus_c2c                    soc:bus_dmc                    passive         null                0    100000000    100000000    400000000
soc:bus_leftbus                null                           simple_ondemand deferrable         50    100000000    100000000    200000000
soc:bus_rightbus               soc:bus_leftbus                passive         null                0    100000000    100000000    200000000
soc:bus_display                soc:bus_leftbus                passive         null                0    160000000    160000000    200000000
soc:bus_fsys                   soc:bus_leftbus                passive         null                0    100000000    100000000    134000000
soc:bus_peri                   soc:bus_leftbus                passive         null                0     50000000     50000000    100000000
soc:bus_mfc                    soc:bus_leftbus                passive         null                0    100000000    100000000    200000000
root@localhost:~# cat /sys/kernel/debug/interconnect/interconnect_graph;
digraph {
        rankdir = LR
        node [shape = record]
        subgraph cluster_1 {
                label = "soc:bus_dmc"
                "2:bus_dmc" [label="2:bus_dmc
                        |avg_bw=0kBps
                        |peak_bw=0kBps"]
        }
        subgraph cluster_2 {
                label = "soc:bus_leftbus"
                "3:bus_leftbus" [label="3:bus_leftbus
                        |avg_bw=0kBps
                        |peak_bw=0kBps"]
        }
        subgraph cluster_3 {
                label = "soc:bus_display"
                "4:bus_display" [label="4:bus_display
                        |avg_bw=0kBps
                        |peak_bw=0kBps"]
        }
        "3:bus_leftbus" -> "2:bus_dmc"
        "4:bus_display" -> "3:bus_leftbus"
}root@localhost:~# cat /sys/kernel/debug/interconnect/interconnect_summary;
 node                                  tag          avg         peak
--------------------------------------------------------------------
bus_dmc                                               0            0
  12c10000.mixer                         0            0            0
bus_leftbus                                           0            0
  12c10000.mixer                         0            0            0
bus_display                                           0            0
  12c10000.mixer                         0            0            0


Thanks,
Chanwoo Choi

> 
> Thanks,
> Georgi
> 
>>
>> PS. The passive governor has a bug related to PM_QOS interface.
>> So, I posted the patch[4].
>>
>>
>> [1] interconnect_graph
>> root@localhost:~# cat /sys/kernel/debug/interconnect/interconnect_graph 
>> digraph {
>>         rankdir = LR
>>         node [shape = record]
>>         subgraph cluster_1 {
>>                 label = "soc:bus_dmc"
>>                 "2:bus_dmc" [label="2:bus_dmc
>>                         |avg_bw=0kBps
>>                         |peak_bw=0kBps"]
>>         }
>>         subgraph cluster_2 {
>>                 label = "soc:bus_leftbus"
>>                 "3:bus_leftbus" [label="3:bus_leftbus
>>                         |avg_bw=0kBps
>>                         |peak_bw=0kBps"]
>>         }
>>         subgraph cluster_3 {
>>                 label = "soc:bus_display"
>>                 "4:bus_display" [label="4:bus_display
>>                         |avg_bw=0kBps
>>                         |peak_bw=0kBps"]
>>         }
>>         "3:bus_leftbus" -> "2:bus_dmc"
>>         "4:bus_display" -> "3:bus_leftbus"
>>
>>
>> [2] interconnect_summary
>> root@localhost:~# cat /sys/kernel/debug/interconnect/interconnect_summary 
>>  node                                  tag          avg         peak
>> --------------------------------------------------------------------
>> bus_dmc                                               0            0
>>   12c10000.mixer                         0            0            0
>> bus_leftbus                                           0            0
>>   12c10000.mixer                         0            0            0
>> bus_display                                           0            0
>>   12c10000.mixer                         0            0            0
>>
>>
>> [3] devfreq_summary
>> root@localhost:~# cat /sys/kernel/debug/devfreq/devfreq_summary 
>> dev                            parent_dev                     governor        timer      polling_ms  cur_freq_Hz  min_freq_Hz  max_freq_Hz
>> ------------------------------ ------------------------------ --------------- ---------- ---------- ------------ ------------ ------------
>> soc:bus_dmc                    null                           simple_ondemand deferrable         50    400000000    400000000    400000000
>> soc:bus_acp                    soc:bus_dmc                    passive         null                0    267000000    100000000    267000000
>> soc:bus_c2c                    soc:bus_dmc                    passive         null                0    400000000    100000000    400000000
>> soc:bus_leftbus                null                           simple_ondemand deferrable         50    200000000    200000000    200000000
>> soc:bus_rightbus               soc:bus_leftbus                passive         null                0    200000000    100000000    200000000
>> soc:bus_display                soc:bus_leftbus                passive         null                0    200000000    200000000    200000000
>> soc:bus_fsys                   soc:bus_leftbus                passive         null                0    134000000    100000000    134000000
>> soc:bus_peri                   soc:bus_leftbus                passive         null                0    100000000     50000000    100000000
>> soc:bus_mfc                    soc:bus_leftbus                passive         null                0    200000000    100000000    200000000
>>
>>
>> [4] PM / devfreq: passive: Update frequency when start governor
>> https://patchwork.kernel.org/project/linux-pm/patch/20201103070646.18687-1-cw00.choi@samsung.com/
>>
>>
> 
>
Sylwester Nawrocki Nov. 3, 2020, 10:12 a.m. UTC | #4
Hi Chanwoo, Georgi

On 03.11.2020 09:53, Chanwoo Choi wrote:
> On 11/3/20 5:29 PM, Georgi Djakov wrote:
>> On 11/3/20 09:54, Chanwoo Choi wrote:

>>> When I tested this patchset on Odroid-U3,
>>> After setting 0 bps by interconnect[1][2],
>>> the frequency of devfreq devs sustain the high frequency
>>> according to the pm qos request.
>>>
>>> So, I try to find the cause of this situation.
>>> In result, it seems that interconnect exynos driver
>>> updates the pm qos request to devfreq device
>>> during the kernel booting. Do you know why the exynos
>>> interconnect driver request the pm qos during probe
>>> without the mixer request?
>>
>> That's probably because of the sync_state support, that was introduced
>> recently. The icc_sync_state callback needs to be added to the driver
>> (i just left a comment on that patch), and then check again if it works.
>>
>> The idea of the sync_state is that there could be multiple users of a
>> path and we must wait for all consumers to tell their bandwidth needs.
>> Otherwise the first consumer may lower the bandwidth or disable a path
>> needed for another consumer (driver), which has not probed yet. So we
>> maintain a floor bandwidth until everyone has probed. By default the floor
>> bandwidth is INT_MAX, but can be overridden by implementing the get_bw()
>> callback.

Thanks for detailed explanation Georgi.

> Thanks for guide. I tested it with your comment of patch2.
> It is well working without problem as I mentioned previously.
> 
> I caught the reset operation of PM QoS requested from interconnect
> on kernel log. In result, after completed the kernel booting,
> there is no pm qos request if hdmi cable is not connected.

Thanks for the bug report Chanwoo, it's related to the sync_state
feature as you guys already figured out.  I had to reorder some code 
in the interconnect driver probe() to avoid some issues, 
i.e. to register PM QoS request before icc_node_add() call but 
I forgot to check initial state of the bus frequencies.

I thought the get_bw implementation might be needed but the default
behaviour seems fine, the PM QoS derived bus frequencies will be 
clamped in the devfreq to valid OPP values.

Chanwoo, in order to set the bandwidth to 0 we could also just blank 
the display. Below are some of the commands I use for testing.

# blank display (disable the mixer entirely)
echo 4 > /sys/devices/platform/exynos-drm/graphics/fb0/blank

# unblank display
echo 0 > /sys/devices/platform/exynos-drm/graphics/fb0/blank

# modetest with 2 planes (higher bandwidth test)
./modetest -s 47:1920x1080 -P 45:1920x1080 -v
Chanwoo Choi Nov. 3, 2020, 10:37 a.m. UTC | #5
Hi Sylwester,

On 11/3/20 7:12 PM, Sylwester Nawrocki wrote:
> Hi Chanwoo, Georgi
> 
> On 03.11.2020 09:53, Chanwoo Choi wrote:
>> On 11/3/20 5:29 PM, Georgi Djakov wrote:
>>> On 11/3/20 09:54, Chanwoo Choi wrote:
> 
>>>> When I tested this patchset on Odroid-U3,
>>>> After setting 0 bps by interconnect[1][2],
>>>> the frequency of devfreq devs sustain the high frequency
>>>> according to the pm qos request.
>>>>
>>>> So, I try to find the cause of this situation.
>>>> In result, it seems that interconnect exynos driver
>>>> updates the pm qos request to devfreq device
>>>> during the kernel booting. Do you know why the exynos
>>>> interconnect driver request the pm qos during probe
>>>> without the mixer request?
>>>
>>> That's probably because of the sync_state support, that was introduced
>>> recently. The icc_sync_state callback needs to be added to the driver
>>> (i just left a comment on that patch), and then check again if it works.
>>>
>>> The idea of the sync_state is that there could be multiple users of a
>>> path and we must wait for all consumers to tell their bandwidth needs.
>>> Otherwise the first consumer may lower the bandwidth or disable a path
>>> needed for another consumer (driver), which has not probed yet. So we
>>> maintain a floor bandwidth until everyone has probed. By default the floor
>>> bandwidth is INT_MAX, but can be overridden by implementing the get_bw()
>>> callback.
> 
> Thanks for detailed explanation Georgi.
> 
>> Thanks for guide. I tested it with your comment of patch2.
>> It is well working without problem as I mentioned previously.
>>
>> I caught the reset operation of PM QoS requested from interconnect
>> on kernel log. In result, after completed the kernel booting,
>> there is no pm qos request if hdmi cable is not connected.
> 
> Thanks for the bug report Chanwoo, it's related to the sync_state
> feature as you guys already figured out.  I had to reorder some code 
> in the interconnect driver probe() to avoid some issues, 
> i.e. to register PM QoS request before icc_node_add() call but 
> I forgot to check initial state of the bus frequencies.
> 
> I thought the get_bw implementation might be needed but the default
> behaviour seems fine, the PM QoS derived bus frequencies will be 
> clamped in the devfreq to valid OPP values.
> 
> Chanwoo, in order to set the bandwidth to 0 we could also just blank 
> the display. Below are some of the commands I use for testing.
> 
> # blank display (disable the mixer entirely)
> echo 4 > /sys/devices/platform/exynos-drm/graphics/fb0/blank
> 
> # unblank display
> echo 0 > /sys/devices/platform/exynos-drm/graphics/fb0/blank
> 
> # modetest with 2 planes (higher bandwidth test)
> ./modetest -s 47:1920x1080 -P 45:1920x1080 -v
> 

Thanks for the test guide.