[v5,0/8] drm: rcar-du: Add Color Management Module (CMM)

Message

Jacopo Mondi Oct. 15, 2019, 10:46 a.m. UTC

References:
A reference to the v1 cover letter, with some background on the CMM is
available here:
https://lkml.org/lkml/2019/6/6/583
v2:
https://lore.kernel.org/linux-renesas-soc/[e-mail]/
v3:
https://lore.kernel.org/linux-renesas-soc/[e-mail]/
v4:
https://lore.kernel.org/linux-renesas-soc/[e-mail]/

Again, quite a consistent changelog, mostly due to the developments happened on
Ezequiel's VOP unit following Sean's advices.

I here implemented the same, and moved the CMM handling to the crtc being and
enable callbacks. As a result the overall implementation results quite a lot
simplified, mostly on the CMM driver side.

I have dropped tags and acks on the CMM driver and CMM enablement patches in
DU crtc driver because of the number of changes.

A more detailed change log:

- Rebased on renesas-devel-2019-10-07-v5.4-rc4

* Bindings/DT
- Included Rob's comments on the yaml file license and the use of 'OneOf'
  in the compatible property description
- Use the bracketed style suggested by Kieran for the 'renesas,cmm' property
  introduced in patch 2
- Re-order the properties in the SoC DTS files as suggested by Kieran

* CMM/DU
- As anticipated, moved CMM management to the crtc from the atomic commit tail
  helper where it was implemented in v4
  This allow to correctly support resume/suspend and proper ordering of the CMM
  enable and setup operations (enable -before- setup)
- As a consequence the CMM driver is greatly simplified by removing the need
  to cache the LUT table entries provided to cmm_setup() and later re-apply
  them at enable time.
- Better support handling of disabled CMM config option by returning -ENODEV
  at cmm_init() time as suggested by Kieran.

* Testing
I have tested by injecting a color inversion LUT table and enabling/disabling it
every 50 displayed frames:
https://jmondi.org/cgit/kmsxx/log/?h=gamma_lut

CMM functionalities are retained between suspend/resume cycles (tested with
suspend-to-idle) without requiring a re-programming of the LUT tables.

Testing with real world use cases might be beneficial. Rajesh are you still
interested in giving this series a spin?

Laurent, Kieran, could we fast-track review of this and hopefully try to have it
merged for v5.5 ?

Thanks Ezequiel for having suggested me this solution.

Thanks
   j

Jacopo Mondi (8):
  dt-bindings: display: renesas,cmm: Add R-Car CMM documentation
  dt-bindings: display, renesas,du: Document cmms property
  drm: rcar-du: Add support for CMM
  drm: rcar-du: kms: Initialize CMM instances
  drm: rcar-du: crtc: Control CMM operations
  drm: rcar-du: crtc: Register GAMMA_LUT properties
  arm64: dts: renesas: Add CMM units to Gen3 SoCs
  drm: rcar-du: kms: Expand comment in vsps parsing routine

 .../bindings/display/renesas,cmm.yaml         |  67 ++++++
 .../bindings/display/renesas,du.txt           |   5 +
 arch/arm64/boot/dts/renesas/r8a7795.dtsi      |  39 ++++
 arch/arm64/boot/dts/renesas/r8a7796.dtsi      |  31 ++-
 arch/arm64/boot/dts/renesas/r8a77965.dtsi     |  31 ++-
 arch/arm64/boot/dts/renesas/r8a77990.dtsi     |  21 ++
 arch/arm64/boot/dts/renesas/r8a77995.dtsi     |  21 ++
 drivers/gpu/drm/rcar-du/Kconfig               |   7 +
 drivers/gpu/drm/rcar-du/Makefile              |   1 +
 drivers/gpu/drm/rcar-du/rcar_cmm.c            | 198 ++++++++++++++++++
 drivers/gpu/drm/rcar-du/rcar_cmm.h            |  60 ++++++
 drivers/gpu/drm/rcar-du/rcar_du_crtc.c        |  89 ++++++++
 drivers/gpu/drm/rcar-du/rcar_du_crtc.h        |   2 +
 drivers/gpu/drm/rcar-du/rcar_du_drv.h         |   2 +
 drivers/gpu/drm/rcar-du/rcar_du_group.c       |   5 +
 drivers/gpu/drm/rcar-du/rcar_du_group.h       |   2 +
 drivers/gpu/drm/rcar-du/rcar_du_kms.c         |  82 +++++++-
 drivers/gpu/drm/rcar-du/rcar_du_regs.h        |   5 +
 18 files changed, 665 insertions(+), 3 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/display/renesas,cmm.yaml
 create mode 100644 drivers/gpu/drm/rcar-du/rcar_cmm.c
 create mode 100644 drivers/gpu/drm/rcar-du/rcar_cmm.h

--
2.23.0

Comments

Laurent Pinchart Oct. 15, 2019, 4:54 p.m. UTC | #1

Hi Jcopo,

Thank you for the patches.

On Tue, Oct 15, 2019 at 12:46:13PM +0200, Jacopo Mondi wrote:
> References:
> A reference to the v1 cover letter, with some background on the CMM is
> available here:
> https://lkml.org/lkml/2019/6/6/583
> v2:
> https://lore.kernel.org/linux-renesas-soc/[e-mail]/
> v3:
> https://lore.kernel.org/linux-renesas-soc/[e-mail]/
> v4:
> https://lore.kernel.org/linux-renesas-soc/[e-mail]/
> 
> Again, quite a consistent changelog, mostly due to the developments happened on
> Ezequiel's VOP unit following Sean's advices.
> 
> I here implemented the same, and moved the CMM handling to the crtc being and
> enable callbacks. As a result the overall implementation results quite a lot
> simplified, mostly on the CMM driver side.
> 
> I have dropped tags and acks on the CMM driver and CMM enablement patches in
> DU crtc driver because of the number of changes.
> 
> A more detailed change log:
> 
> - Rebased on renesas-devel-2019-10-07-v5.4-rc4
> 
> * Bindings/DT
> - Included Rob's comments on the yaml file license and the use of 'OneOf'
>   in the compatible property description
> - Use the bracketed style suggested by Kieran for the 'renesas,cmm' property
>   introduced in patch 2
> - Re-order the properties in the SoC DTS files as suggested by Kieran
> 
> * CMM/DU
> - As anticipated, moved CMM management to the crtc from the atomic commit tail
>   helper where it was implemented in v4
>   This allow to correctly support resume/suspend and proper ordering of the CMM
>   enable and setup operations (enable -before- setup)
> - As a consequence the CMM driver is greatly simplified by removing the need
>   to cache the LUT table entries provided to cmm_setup() and later re-apply
>   them at enable time.
> - Better support handling of disabled CMM config option by returning -ENODEV
>   at cmm_init() time as suggested by Kieran.

Could you, for your next series (hopefully not CMM-related :-)) move the
changelog to individual patches ? Having it in the cover letter requires
going back and forth, and also doesn't provide detailed changelog. I
recommend adding below a --- line in the commit message, so that every
time you commit --amend to update a patch, you can record the exact
detailed change at the same time.

> * Testing
> I have tested by injecting a color inversion LUT table and enabling/disabling it
> every 50 displayed frames:
> https://jmondi.org/cgit/kmsxx/log/?h=gamma_lut
> 
> CMM functionalities are retained between suspend/resume cycles (tested with
> suspend-to-idle) without requiring a re-programming of the LUT tables.
> 
> Testing with real world use cases might be beneficial. Rajesh are you still
> interested in giving this series a spin?
> 
> Laurent, Kieran, could we fast-track review of this and hopefully try to have it
> merged for v5.5 ?
> 
> Thanks Ezequiel for having suggested me this solution.
> 
> Thanks
>    j
> 
> Jacopo Mondi (8):
>   dt-bindings: display: renesas,cmm: Add R-Car CMM documentation
>   dt-bindings: display, renesas,du: Document cmms property
>   drm: rcar-du: Add support for CMM
>   drm: rcar-du: kms: Initialize CMM instances
>   drm: rcar-du: crtc: Control CMM operations
>   drm: rcar-du: crtc: Register GAMMA_LUT properties
>   arm64: dts: renesas: Add CMM units to Gen3 SoCs
>   drm: rcar-du: kms: Expand comment in vsps parsing routine
> 
>  .../bindings/display/renesas,cmm.yaml         |  67 ++++++
>  .../bindings/display/renesas,du.txt           |   5 +
>  arch/arm64/boot/dts/renesas/r8a7795.dtsi      |  39 ++++
>  arch/arm64/boot/dts/renesas/r8a7796.dtsi      |  31 ++-
>  arch/arm64/boot/dts/renesas/r8a77965.dtsi     |  31 ++-
>  arch/arm64/boot/dts/renesas/r8a77990.dtsi     |  21 ++
>  arch/arm64/boot/dts/renesas/r8a77995.dtsi     |  21 ++
>  drivers/gpu/drm/rcar-du/Kconfig               |   7 +
>  drivers/gpu/drm/rcar-du/Makefile              |   1 +
>  drivers/gpu/drm/rcar-du/rcar_cmm.c            | 198 ++++++++++++++++++
>  drivers/gpu/drm/rcar-du/rcar_cmm.h            |  60 ++++++
>  drivers/gpu/drm/rcar-du/rcar_du_crtc.c        |  89 ++++++++
>  drivers/gpu/drm/rcar-du/rcar_du_crtc.h        |   2 +
>  drivers/gpu/drm/rcar-du/rcar_du_drv.h         |   2 +
>  drivers/gpu/drm/rcar-du/rcar_du_group.c       |   5 +
>  drivers/gpu/drm/rcar-du/rcar_du_group.h       |   2 +
>  drivers/gpu/drm/rcar-du/rcar_du_kms.c         |  82 +++++++-
>  drivers/gpu/drm/rcar-du/rcar_du_regs.h        |   5 +
>  18 files changed, 665 insertions(+), 3 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/display/renesas,cmm.yaml
>  create mode 100644 drivers/gpu/drm/rcar-du/rcar_cmm.c
>  create mode 100644 drivers/gpu/drm/rcar-du/rcar_cmm.h
>

Kalakodima Venkata Rajesh (RBEI/ECF3) Nov. 11, 2019, 11:21 a.m. UTC | #2

Hi Jacopo,

Please find comments below.

Best regards,

Rajesh Kv
RBEI/ECF3  

> -----Original Message-----
> From: [e-mail] <linux-kernel-
> [e-mail]> On Behalf Of Jacopo Mondi
> Sent: Tuesday, October 15, 2019 4:16 PM
> To: [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail]; Kalakodima Venkata Rajesh
> (RBEI/ECF3) <[e-mail]>
> Cc: Jacopo Mondi <[e-mail]>; [e-mail];
> [e-mail]; [e-mail]; [e-mail]; Harsha
> Manjula Mallikarjun (RBEI/ECF3) <[e-mail]>;
> [e-mail]; [e-mail]; linux-renesas-
> [e-mail]; [e-mail]; linux-
> [e-mail]
> Subject: [PATCH v5 0/8] drm: rcar-du: Add Color Management Module (CMM)
> 
> References:
> A reference to the v1 cover letter, with some background on the CMM is
> available here:
> https://lkml.org/lkml/2019/6/6/583
> v2:
> https://lore.kernel.org/linux-renesas-soc/20190706140746.29132-10-
> [e-mail]/
> v3:
> https://lore.kernel.org/linux-renesas-soc/20190825135154.11488-1-
> [e-mail]/
> v4:
> https://lore.kernel.org/linux-renesas-soc/20190906135436.10622-1-
> [e-mail]/
> 
> Again, quite a consistent changelog, mostly due to the developments happened
> on Ezequiel's VOP unit following Sean's advices.
> 
> I here implemented the same, and moved the CMM handling to the crtc being
> and enable callbacks. As a result the overall implementation results quite a lot
> simplified, mostly on the CMM driver side.
> 
> I have dropped tags and acks on the CMM driver and CMM enablement patches
> in DU crtc driver because of the number of changes.
> 
> A more detailed change log:
> 
> - Rebased on renesas-devel-2019-10-07-v5.4-rc4
> 
> * Bindings/DT
> - Included Rob's comments on the yaml file license and the use of 'OneOf'
>   in the compatible property description
> - Use the bracketed style suggested by Kieran for the 'renesas,cmm' property
>   introduced in patch 2
> - Re-order the properties in the SoC DTS files as suggested by Kieran
> 
> * CMM/DU
> - As anticipated, moved CMM management to the crtc from the atomic commit
> tail
>   helper where it was implemented in v4
>   This allow to correctly support resume/suspend and proper ordering of the
> CMM
>   enable and setup operations (enable -before- setup)
> - As a consequence the CMM driver is greatly simplified by removing the need
>   to cache the LUT table entries provided to cmm_setup() and later re-apply
>   them at enable time.
> - Better support handling of disabled CMM config option by returning -ENODEV
>   at cmm_init() time as suggested by Kieran.
> 
> * Testing
> I have tested by injecting a color inversion LUT table and enabling/disabling it
> every 50 displayed frames:
> https://jmondi.org/cgit/kmsxx/log/?h=gamma_lut
> 
> CMM functionalities are retained between suspend/resume cycles (tested with
> suspend-to-idle) without requiring a re-programming of the LUT tables.
> 
> Testing with real world use cases might be beneficial. Rajesh are you still
> interested in giving this series a spin

I have tested version v3 of CMM module with a demo application based on libdrm 
library. I could successfully test setting of Gamma LUT.

Next step is to test on full featured graphics stack i.e. involving Weston and OpenGL.
Weston can set Gamma. I have to stop this work for a while due to other high prio activities.
I plan to resume soon.

> 
> Laurent, Kieran, could we fast-track review of this and hopefully try to have it
> merged for v5.5 ?
> 
> Thanks Ezequiel for having suggested me this solution.
> 
> Thanks
>    j
> 
> Jacopo Mondi (8):
>   dt-bindings: display: renesas,cmm: Add R-Car CMM documentation
>   dt-bindings: display, renesas,du: Document cmms property
>   drm: rcar-du: Add support for CMM
>   drm: rcar-du: kms: Initialize CMM instances
>   drm: rcar-du: crtc: Control CMM operations
>   drm: rcar-du: crtc: Register GAMMA_LUT properties
>   arm64: dts: renesas: Add CMM units to Gen3 SoCs
>   drm: rcar-du: kms: Expand comment in vsps parsing routine
> 
>  .../bindings/display/renesas,cmm.yaml         |  67 ++++++
>  .../bindings/display/renesas,du.txt           |   5 +
>  arch/arm64/boot/dts/renesas/r8a7795.dtsi      |  39 ++++
>  arch/arm64/boot/dts/renesas/r8a7796.dtsi      |  31 ++-
>  arch/arm64/boot/dts/renesas/r8a77965.dtsi     |  31 ++-
>  arch/arm64/boot/dts/renesas/r8a77990.dtsi     |  21 ++
>  arch/arm64/boot/dts/renesas/r8a77995.dtsi     |  21 ++
>  drivers/gpu/drm/rcar-du/Kconfig               |   7 +
>  drivers/gpu/drm/rcar-du/Makefile              |   1 +
>  drivers/gpu/drm/rcar-du/rcar_cmm.c            | 198 ++++++++++++++++++
>  drivers/gpu/drm/rcar-du/rcar_cmm.h            |  60 ++++++
>  drivers/gpu/drm/rcar-du/rcar_du_crtc.c        |  89 ++++++++
>  drivers/gpu/drm/rcar-du/rcar_du_crtc.h        |   2 +
>  drivers/gpu/drm/rcar-du/rcar_du_drv.h         |   2 +
>  drivers/gpu/drm/rcar-du/rcar_du_group.c       |   5 +
>  drivers/gpu/drm/rcar-du/rcar_du_group.h       |   2 +
>  drivers/gpu/drm/rcar-du/rcar_du_kms.c         |  82 +++++++-
>  drivers/gpu/drm/rcar-du/rcar_du_regs.h        |   5 +
>  18 files changed, 665 insertions(+), 3 deletions(-)  create mode 100644
> Documentation/devicetree/bindings/display/renesas,cmm.yaml
>  create mode 100644 drivers/gpu/drm/rcar-du/rcar_cmm.c
>  create mode 100644 drivers/gpu/drm/rcar-du/rcar_cmm.h
> 
> --
> 2.23.0

Jacopo Mondi Nov. 11, 2019, 1:06 p.m. UTC | #3

Hello,

On Mon, Nov 11, 2019 at 11:21:28AM +0000, Kalakodima Venkata Rajesh (RBEI/ECF3) wrote:
> Hi Jacopo,
>
> Please find comments below.
>
> Best regards,
>
> Rajesh Kv
> RBEI/ECF3
>
> > -----Original Message-----
> > From: [e-mail] <linux-kernel-
> > [e-mail]> On Behalf Of Jacopo Mondi
> > Sent: Tuesday, October 15, 2019 4:16 PM
> > To: [e-mail];
> > [e-mail]; [e-mail];
> > [e-mail]; [e-mail]; Kalakodima Venkata Rajesh
> > (RBEI/ECF3) <[e-mail]>
> > Cc: Jacopo Mondi <[e-mail]>; [e-mail];
> > [e-mail]; [e-mail]; [e-mail]; Harsha
> > Manjula Mallikarjun (RBEI/ECF3) <[e-mail]>;
> > [e-mail]; [e-mail]; linux-renesas-
> > [e-mail]; [e-mail]; linux-
> > [e-mail]
> > Subject: [PATCH v5 0/8] drm: rcar-du: Add Color Management Module (CMM)
> >
> > References:
> > A reference to the v1 cover letter, with some background on the CMM is
> > available here:
> > https://lkml.org/lkml/2019/6/6/583
> > v2:
> > https://lore.kernel.org/linux-renesas-soc/20190706140746.29132-10-
> > [e-mail]/
> > v3:
> > https://lore.kernel.org/linux-renesas-soc/20190825135154.11488-1-
> > [e-mail]/
> > v4:
> > https://lore.kernel.org/linux-renesas-soc/20190906135436.10622-1-
> > [e-mail]/
> >
> > Again, quite a consistent changelog, mostly due to the developments happened
> > on Ezequiel's VOP unit following Sean's advices.
> >
> > I here implemented the same, and moved the CMM handling to the crtc being
> > and enable callbacks. As a result the overall implementation results quite a lot
> > simplified, mostly on the CMM driver side.
> >
> > I have dropped tags and acks on the CMM driver and CMM enablement patches
> > in DU crtc driver because of the number of changes.
> >
> > A more detailed change log:
> >
> > - Rebased on renesas-devel-2019-10-07-v5.4-rc4
> >
> > * Bindings/DT
> > - Included Rob's comments on the yaml file license and the use of 'OneOf'
> >   in the compatible property description
> > - Use the bracketed style suggested by Kieran for the 'renesas,cmm' property
> >   introduced in patch 2
> > - Re-order the properties in the SoC DTS files as suggested by Kieran
> >
> > * CMM/DU
> > - As anticipated, moved CMM management to the crtc from the atomic commit
> > tail
> >   helper where it was implemented in v4
> >   This allow to correctly support resume/suspend and proper ordering of the
> > CMM
> >   enable and setup operations (enable -before- setup)
> > - As a consequence the CMM driver is greatly simplified by removing the need
> >   to cache the LUT table entries provided to cmm_setup() and later re-apply
> >   them at enable time.
> > - Better support handling of disabled CMM config option by returning -ENODEV
> >   at cmm_init() time as suggested by Kieran.
> >
> > * Testing
> > I have tested by injecting a color inversion LUT table and enabling/disabling it
> > every 50 displayed frames:
> > https://jmondi.org/cgit/kmsxx/log/?h=gamma_lut
> >
> > CMM functionalities are retained between suspend/resume cycles (tested with
> > suspend-to-idle) without requiring a re-programming of the LUT tables.
> >
> > Testing with real world use cases might be beneficial. Rajesh are you still
> > interested in giving this series a spin
>
> I have tested version v3 of CMM module with a demo application based on libdrm
> library. I could successfully test setting of Gamma LUT.

\o/

If you want to, please send your Tested-by tag, so that it can be
collected, as CMM support will be collected for the v5.6 merge window, as we
had a small issue that prevented v6 from being part of the v5.5 one.

>
> Next step is to test on full featured graphics stack i.e. involving Weston and OpenGL.
> Weston can set Gamma. I have to stop this work for a while due to other high prio activities.
> I plan to resume soon.
>

Thanks for testing and please keep us posted!

Thanks
   j

> >
> > Laurent, Kieran, could we fast-track review of this and hopefully try to have it
> > merged for v5.5 ?
> >
> > Thanks Ezequiel for having suggested me this solution.
> >
> > Thanks
> >    j
> >
> > Jacopo Mondi (8):
> >   dt-bindings: display: renesas,cmm: Add R-Car CMM documentation
> >   dt-bindings: display, renesas,du: Document cmms property
> >   drm: rcar-du: Add support for CMM
> >   drm: rcar-du: kms: Initialize CMM instances
> >   drm: rcar-du: crtc: Control CMM operations
> >   drm: rcar-du: crtc: Register GAMMA_LUT properties
> >   arm64: dts: renesas: Add CMM units to Gen3 SoCs
> >   drm: rcar-du: kms: Expand comment in vsps parsing routine
> >
> >  .../bindings/display/renesas,cmm.yaml         |  67 ++++++
> >  .../bindings/display/renesas,du.txt           |   5 +
> >  arch/arm64/boot/dts/renesas/r8a7795.dtsi      |  39 ++++
> >  arch/arm64/boot/dts/renesas/r8a7796.dtsi      |  31 ++-
> >  arch/arm64/boot/dts/renesas/r8a77965.dtsi     |  31 ++-
> >  arch/arm64/boot/dts/renesas/r8a77990.dtsi     |  21 ++
> >  arch/arm64/boot/dts/renesas/r8a77995.dtsi     |  21 ++
> >  drivers/gpu/drm/rcar-du/Kconfig               |   7 +
> >  drivers/gpu/drm/rcar-du/Makefile              |   1 +
> >  drivers/gpu/drm/rcar-du/rcar_cmm.c            | 198 ++++++++++++++++++
> >  drivers/gpu/drm/rcar-du/rcar_cmm.h            |  60 ++++++
> >  drivers/gpu/drm/rcar-du/rcar_du_crtc.c        |  89 ++++++++
> >  drivers/gpu/drm/rcar-du/rcar_du_crtc.h        |   2 +
> >  drivers/gpu/drm/rcar-du/rcar_du_drv.h         |   2 +
> >  drivers/gpu/drm/rcar-du/rcar_du_group.c       |   5 +
> >  drivers/gpu/drm/rcar-du/rcar_du_group.h       |   2 +
> >  drivers/gpu/drm/rcar-du/rcar_du_kms.c         |  82 +++++++-
> >  drivers/gpu/drm/rcar-du/rcar_du_regs.h        |   5 +
> >  18 files changed, 665 insertions(+), 3 deletions(-)  create mode 100644
> > Documentation/devicetree/bindings/display/renesas,cmm.yaml
> >  create mode 100644 drivers/gpu/drm/rcar-du/rcar_cmm.c
> >  create mode 100644 drivers/gpu/drm/rcar-du/rcar_cmm.h
> >
> > --
> > 2.23.0
>

Eugeniu Rosca May 27, 2020, 7:15 a.m. UTC | #4

Hi Jacopo,

On Tue, Oct 15, 2019 at 12:46:13PM +0200, Jacopo Mondi wrote:
 ----8<---

> * Testing
> I have tested by injecting a color inversion LUT table and enabling/disabling it
> every 50 displayed frames:
> https://jmondi.org/cgit/kmsxx/log/?h=gamma_lut

Could you kindly share the cross compilation steps for your kmsxx fork?

Just out of curiosity, have you ever tried to pull the display's HDMI
cable while reading from CM2_LUT_TBL?

At least with the out-of-tree CMM implementation [*], this sends the
R-Car3 reference targets into an unrecoverable freeze, with no lockup
reported by the kernel (i.e. looks like an serious HW issue).

> 
> CMM functionalities are retained between suspend/resume cycles (tested with
> suspend-to-idle) without requiring a re-programming of the LUT tables.

Hmm. Is this backed up by any statement in the HW User's manual?
This comes in contrast with the original Renesas CMM implementation [**]
which does make use of suspend (where the freeze actually happens).

Can we infer, based on your statement, that we could also get rid of
the suspend callback in [**]?

[*] https://github.com/renesas-rcar/du_cmm
[**] https://github.com/renesas-rcar/du_cmm/blob/c393ed49834bdbc/meta-rcar-gen3/recipes-kernel/linux/linux-renesas/0001-drm-rcar-du-Add-DU-CMM-support.patch#L1912

Geert Uytterhoeven May 27, 2020, 7:34 a.m. UTC | #5

Hi Eugeniu,

On Wed, May 27, 2020 at 9:16 AM Eugeniu Rosca <[e-mail]> wrote:
> On Tue, Oct 15, 2019 at 12:46:13PM +0200, Jacopo Mondi wrote:
> > CMM functionalities are retained between suspend/resume cycles (tested with
> > suspend-to-idle) without requiring a re-programming of the LUT tables.
>
> Hmm. Is this backed up by any statement in the HW User's manual?
> This comes in contrast with the original Renesas CMM implementation [**]
> which does make use of suspend (where the freeze actually happens).
>
> Can we infer, based on your statement, that we could also get rid of
> the suspend callback in [**]?

While the CMM state will be retained across suspend-to-idle, I'm quite
sure it will be lost by suspend-to-RAM, at least on the Salvator-X(S),
ULCB, and Ebisu development boards, as PSCI will ask the BD9571WMV
regulator to power down the R-Car SoC.

So IMHO we do need suspend/resume handling.

Gr{oetje,eeting}s,

                        Geert

Gotthard Voellmeke May 27, 2020, 7:40 a.m. UTC | #6

Added Hien-san, Michael K. from Renesas.

-----Original Message-----
From: Geert Uytterhoeven <[e-mail]>
Sent: Mittwoch, 27. Mai 2020 09:35
To: REE [e-mail] <[e-mail]>
Cc: Jacopo Mondi <[e-mail]>; Laurent Pinchart <[e-mail]>; Kieran Bingham <[e-mail]>; Simon Horman <[e-mail]>; Ulrich Hecht <[e-mail]>; [e-mail]; David Airlie <[e-mail]>; Daniel Vetter <[e-mail]>; KOJI MATSUOKA <[e-mail]>; [e-mail]; [e-mail]; Ezequiel Garcia <[e-mail]>; Sean Paul <[e-mail]>; Linux-Renesas <[e-mail]>; DRI Development <[e-mail]>; Linux Kernel Mailing List <[e-mail]>; Michael Dege <[e-mail]>; Gotthard Voellmeke <[e-mail]>; REE [e-mail] <[e-mail]>; Michael Rodin <[e-mail]>; [e-mail]; Eugeniu Rosca <[e-mail]>
Subject: Re: [PATCH v5 0/8] drm: rcar-du: Add Color Management Module (CMM)

Hi Eugeniu,

On Wed, May 27, 2020 at 9:16 AM Eugeniu Rosca <[e-mail]> wrote:
> On Tue, Oct 15, 2019 at 12:46:13PM +0200, Jacopo Mondi wrote:
> > CMM functionalities are retained between suspend/resume cycles (tested with
> > suspend-to-idle) without requiring a re-programming of the LUT tables.
>
> Hmm. Is this backed up by any statement in the HW User's manual?
> This comes in contrast with the original Renesas CMM implementation [**]
> which does make use of suspend (where the freeze actually happens).
>
> Can we infer, based on your statement, that we could also get rid of
> the suspend callback in [**]?

While the CMM state will be retained across suspend-to-idle, I'm quite
sure it will be lost by suspend-to-RAM, at least on the Salvator-X(S),
ULCB, and Ebisu development boards, as PSCI will ask the BD9571WMV
regulator to power down the R-Car SoC.

So IMHO we do need suspend/resume handling.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [e-mail]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds


Renesas Electronics Europe GmbH, Geschaeftsfuehrer/President: Carsten Jauch, Sitz der Gesellschaft/Registered office: Duesseldorf, Arcadiastrasse 10, 40472 Duesseldorf, Germany, Handelsregister/Commercial Register: Duesseldorf, HRB 3708 USt-IDNr./Tax identification no.: DE 119353406 WEEE-Reg.-Nr./WEEE reg. no.: DE 14978647

Eugeniu Rosca May 27, 2020, 7:44 a.m. UTC | #7

Hi Geert,

On Wed, May 27, 2020 at 09:34:30AM +0200, Geert Uytterhoeven wrote:
> On Wed, May 27, 2020 at 9:16 AM Eugeniu Rosca <[e-mail]> wrote:
> > On Tue, Oct 15, 2019 at 12:46:13PM +0200, Jacopo Mondi wrote:
> > > CMM functionalities are retained between suspend/resume cycles (tested with
> > > suspend-to-idle) without requiring a re-programming of the LUT tables.
> >
> > Hmm. Is this backed up by any statement in the HW User's manual?
> > This comes in contrast with the original Renesas CMM implementation [**]
> > which does make use of suspend (where the freeze actually happens).
> >
> > Can we infer, based on your statement, that we could also get rid of
> > the suspend callback in [**]?
> 
> While the CMM state will be retained across suspend-to-idle, I'm quite
> sure it will be lost by suspend-to-RAM, at least on the Salvator-X(S),
> ULCB, and Ebisu development boards, as PSCI will ask the BD9571WMV
> regulator to power down the R-Car SoC.
> 
> So IMHO we do need suspend/resume handling.

That makes sense. I should be more careful about suspend-to-idle
vs suspend-to-ram and not alias the two.

Jacopo Mondi June 5, 2020, 1:29 p.m. UTC | #8

Hello Eugeniu,
   sorry for the late reply

On Wed, May 27, 2020 at 09:15:55AM +0200, Eugeniu Rosca wrote:
> Hi Jacopo,
>
> On Tue, Oct 15, 2019 at 12:46:13PM +0200, Jacopo Mondi wrote:
>  ----8<---
>
> > * Testing
> > I have tested by injecting a color inversion LUT table and enabling/disabling it
> > every 50 displayed frames:
> > https://jmondi.org/cgit/kmsxx/log/?h=gamma_lut
>
> Could you kindly share the cross compilation steps for your kmsxx fork?
>

I usually build it on the target :)

> Just out of curiosity, have you ever tried to pull the display's HDMI
> cable while reading from CM2_LUT_TBL?

Ahem, not really :) Did I get you right, you mean disconnecting the
HDMI cable from the board ?

>
> At least with the out-of-tree CMM implementation [*], this sends the
> R-Car3 reference targets into an unrecoverable freeze, with no lockup
> reported by the kernel (i.e. looks like an serious HW issue).
>
> >
> > CMM functionalities are retained between suspend/resume cycles (tested with
> > suspend-to-idle) without requiring a re-programming of the LUT tables.
>
> Hmm. Is this backed up by any statement in the HW User's manual?
> This comes in contrast with the original Renesas CMM implementation [**]
> which does make use of suspend (where the freeze actually happens).
>
> Can we infer, based on your statement, that we could also get rid of
> the suspend callback in [**]?

As Geert (thanks) explained what I've tested with is suspend-to-idle,
which retains the state of the LUT tables (and I assume other
not-yet-implemented CMM features, like CLU). I recall the out-of-tree
driver has suspend/resume routines but I never really tested that.

Thanks
   j

>
> [*] https://github.com/renesas-rcar/du_cmm
> [**] https://github.com/renesas-rcar/du_cmm/blob/c393ed49834bdbc/meta-rcar-gen3/recipes-kernel/linux/linux-renesas/0001-drm-rcar-du-Add-DU-CMM-support.patch#L1912
>
> --
> Best regards,
> Eugeniu Rosca

Eugeniu Rosca June 5, 2020, 1:41 p.m. UTC | #9

Hi Jacopo,

On Fri, Jun 05, 2020 at 03:29:00PM +0200, Jacopo Mondi wrote:
> On Wed, May 27, 2020 at 09:15:55AM +0200, Eugeniu Rosca wrote:
> > Could you kindly share the cross compilation steps for your kmsxx fork?
> 
> I usually build it on the target :)

Interesting approach. With ARM getting more and more potent, why not? :)

> 
> > Just out of curiosity, have you ever tried to pull the display's HDMI
> > cable while reading from CM2_LUT_TBL?
> 
> Ahem, not really :) Did I get you right, you mean disconnecting the
> HDMI cable from the board ?

Right.

> >
> > At least with the out-of-tree CMM implementation [*], this sends the
> > R-Car3 reference targets into an unrecoverable freeze, with no lockup
> > reported by the kernel (i.e. looks like an serious HW issue).
> >
> > >
> > > CMM functionalities are retained between suspend/resume cycles (tested with
> > > suspend-to-idle) without requiring a re-programming of the LUT tables.
> >
> > Hmm. Is this backed up by any statement in the HW User's manual?
> > This comes in contrast with the original Renesas CMM implementation [**]
> > which does make use of suspend (where the freeze actually happens).
> >
> > Can we infer, based on your statement, that we could also get rid of
> > the suspend callback in [**]?
> 
> As Geert (thanks) explained what I've tested with is suspend-to-idle,
> which retains the state of the LUT tables (and I assume other
> not-yet-implemented CMM features, like CLU). I recall the out-of-tree
> driver has suspend/resume routines but I never really tested that.

I see. JFYI, there is a flaw in the suspend handling in the out-of-tree
CMM patch [*], which renders the SoC unresponsive on HDMI hotplug. The
fix is currently under review. Hopefully it will make its way to [*]
in the nearest future. Just to keep in mind for the moment when CMM
s2ram will become a mainline feature.

> >
> > [*] https://github.com/renesas-rcar/du_cmm
> > [**] https://github.com/renesas-rcar/du_cmm/blob/c393ed49834bdbc/meta-rcar-gen3/recipes-kernel/linux/linux-renesas/0001-drm-rcar-du-Add-DU-CMM-support.patch#L1912

Jacopo Mondi June 5, 2020, 1:53 p.m. UTC | #10

Hi Eugeniu

On Fri, Jun 05, 2020 at 03:41:24PM +0200, Eugeniu Rosca wrote:
> Hi Jacopo,
>
> On Fri, Jun 05, 2020 at 03:29:00PM +0200, Jacopo Mondi wrote:
> > On Wed, May 27, 2020 at 09:15:55AM +0200, Eugeniu Rosca wrote:
> > > Could you kindly share the cross compilation steps for your kmsxx fork?
> >
> > I usually build it on the target :)
>
> Interesting approach. With ARM getting more and more potent, why not? :)
>

For 'small' utilities like kmsxx it's doable

> >
> > > Just out of curiosity, have you ever tried to pull the display's HDMI
> > > cable while reading from CM2_LUT_TBL?
> >
> > Ahem, not really :) Did I get you right, you mean disconnecting the
> > HDMI cable from the board ?
>
> Right.
>

So, no, I have not tried. Do you see any intersting failure with the
mainline version ?

> > >
> > > At least with the out-of-tree CMM implementation [*], this sends the
> > > R-Car3 reference targets into an unrecoverable freeze, with no lockup
> > > reported by the kernel (i.e. looks like an serious HW issue).
> > >
> > > >
> > > > CMM functionalities are retained between suspend/resume cycles (tested with
> > > > suspend-to-idle) without requiring a re-programming of the LUT tables.
> > >
> > > Hmm. Is this backed up by any statement in the HW User's manual?
> > > This comes in contrast with the original Renesas CMM implementation [**]
> > > which does make use of suspend (where the freeze actually happens).
> > >
> > > Can we infer, based on your statement, that we could also get rid of
> > > the suspend callback in [**]?
> >
> > As Geert (thanks) explained what I've tested with is suspend-to-idle,
> > which retains the state of the LUT tables (and I assume other
> > not-yet-implemented CMM features, like CLU). I recall the out-of-tree
> > driver has suspend/resume routines but I never really tested that.
>
> I see. JFYI, there is a flaw in the suspend handling in the out-of-tree
> CMM patch [*], which renders the SoC unresponsive on HDMI hotplug. The
> fix is currently under review. Hopefully it will make its way to [*]
> in the nearest future. Just to keep in mind for the moment when CMM
> s2ram will become a mainline feature.

Thanks, let's keep this in mind. Next week I'll run a few tests again
with s2ram and will get back to you.

Thanks
  j

>
> > >
> > > [*] https://github.com/renesas-rcar/du_cmm
> > > [**] https://github.com/renesas-rcar/du_cmm/blob/c393ed49834bdbc/meta-rcar-gen3/recipes-kernel/linux/linux-renesas/0001-drm-rcar-du-Add-DU-CMM-support.patch#L1912
>
> --
> Best regards,
> Eugeniu Rosca

Laurent Pinchart June 5, 2020, 7:05 p.m. UTC | #11

On Fri, Jun 05, 2020 at 03:29:00PM +0200, Jacopo Mondi wrote:
> Hello Eugeniu,
>    sorry for the late reply
> 
> On Wed, May 27, 2020 at 09:15:55AM +0200, Eugeniu Rosca wrote:
> > Hi Jacopo,
> >
> > On Tue, Oct 15, 2019 at 12:46:13PM +0200, Jacopo Mondi wrote:
> >  ----8<---
> >
> > > * Testing
> > > I have tested by injecting a color inversion LUT table and enabling/disabling it
> > > every 50 displayed frames:
> > > https://jmondi.org/cgit/kmsxx/log/?h=gamma_lut
> >
> > Could you kindly share the cross compilation steps for your kmsxx fork?
> 
> I usually build it on the target :)

Otherwise, cross-compilation instructions are available in README.md.

$ mkdir build
$ cd build
$ cmake -DCMAKE_TOOLCHAIN_FILE=<buildrootpath>/output/host/usr/share/buildroot/toolchainfile.cmake ..
$ make -j4

I assume you may use yocto instead of buildroot, you would have to
locate the toolchainfile.cmake file accordingly.

> > Just out of curiosity, have you ever tried to pull the display's HDMI
> > cable while reading from CM2_LUT_TBL?
> 
> Ahem, not really :) Did I get you right, you mean disconnecting the
> HDMI cable from the board ?
> 
> > At least with the out-of-tree CMM implementation [*], this sends the
> > R-Car3 reference targets into an unrecoverable freeze, with no lockup
> > reported by the kernel (i.e. looks like an serious HW issue).
> >
> > >
> > > CMM functionalities are retained between suspend/resume cycles (tested with
> > > suspend-to-idle) without requiring a re-programming of the LUT tables.
> >
> > Hmm. Is this backed up by any statement in the HW User's manual?
> > This comes in contrast with the original Renesas CMM implementation [**]
> > which does make use of suspend (where the freeze actually happens).
> >
> > Can we infer, based on your statement, that we could also get rid of
> > the suspend callback in [**]?
> 
> As Geert (thanks) explained what I've tested with is suspend-to-idle,
> which retains the state of the LUT tables (and I assume other
> not-yet-implemented CMM features, like CLU). I recall the out-of-tree
> driver has suspend/resume routines but I never really tested that.
> 
> > [*] https://github.com/renesas-rcar/du_cmm
> > [**] https://github.com/renesas-rcar/du_cmm/blob/c393ed49834bdbc/meta-rcar-gen3/recipes-kernel/linux/linux-renesas/0001-drm-rcar-du-Add-DU-CMM-support.patch#L1912

Laurent Pinchart June 7, 2020, 2:41 a.m. UTC | #12

Hello,

On Fri, Jun 05, 2020 at 03:53:15PM +0200, Jacopo Mondi wrote:
> On Fri, Jun 05, 2020 at 03:41:24PM +0200, Eugeniu Rosca wrote:
> > On Fri, Jun 05, 2020 at 03:29:00PM +0200, Jacopo Mondi wrote:
> >> On Wed, May 27, 2020 at 09:15:55AM +0200, Eugeniu Rosca wrote:
> >>> Could you kindly share the cross compilation steps for your kmsxx fork?
> >>
> >> I usually build it on the target :)
> >
> > Interesting approach. With ARM getting more and more potent, why not? :)
> 
> For 'small' utilities like kmsxx it's doable
> 
> >>> Just out of curiosity, have you ever tried to pull the display's HDMI
> >>> cable while reading from CM2_LUT_TBL?
> >>
> >> Ahem, not really :) Did I get you right, you mean disconnecting the
> >> HDMI cable from the board ?
> >
> > Right.
> 
> So, no, I have not tried. Do you see any intersting failure with the
> mainline version ?

Jacopo, would you be able to give this a try ?

> >>> At least with the out-of-tree CMM implementation [*], this sends the
> >>> R-Car3 reference targets into an unrecoverable freeze, with no lockup
> >>> reported by the kernel (i.e. looks like an serious HW issue).
> >>>
> >>>> CMM functionalities are retained between suspend/resume cycles (tested with
> >>>> suspend-to-idle) without requiring a re-programming of the LUT tables.
> >>>
> >>> Hmm. Is this backed up by any statement in the HW User's manual?
> >>> This comes in contrast with the original Renesas CMM implementation [**]
> >>> which does make use of suspend (where the freeze actually happens).
> >>>
> >>> Can we infer, based on your statement, that we could also get rid of
> >>> the suspend callback in [**]?
> >>
> >> As Geert (thanks) explained what I've tested with is suspend-to-idle,
> >> which retains the state of the LUT tables (and I assume other
> >> not-yet-implemented CMM features, like CLU). I recall the out-of-tree
> >> driver has suspend/resume routines but I never really tested that.
> >
> > I see. JFYI, there is a flaw in the suspend handling in the out-of-tree
> > CMM patch [*], which renders the SoC unresponsive on HDMI hotplug. The
> > fix is currently under review. Hopefully it will make its way to [*]
> > in the nearest future. Just to keep in mind for the moment when CMM
> > s2ram will become a mainline feature.
> 
> Thanks, let's keep this in mind. Next week I'll run a few tests again
> with s2ram and will get back to you.

Note that the CMM driver is controlled by the DU driver. As the DU
driver will reenable the display during resume, it will call
rcar_du_cmm_setup() at resume time, which will reprogram the CMM. There
should thus be no need for manual suspend/resume handling in the CMM as
far as I can tell, but we need to ensure that the CMM is suspended
before and resumed after the DU. I believe this could be implemented
using device links.

> >>> [*] https://github.com/renesas-rcar/du_cmm
> >>> [**] https://github.com/renesas-rcar/du_cmm/blob/c393ed49834bdbc/meta-rcar-gen3/recipes-kernel/linux/linux-renesas/0001-drm-rcar-du-Add-DU-CMM-support.patch#L1912

Eugeniu Rosca June 8, 2020, 9:44 a.m. UTC | #13

Hello,

Many thanks for your comments and involvement.

On Sun, Jun 07, 2020 at 05:41:58AM +0300, Laurent Pinchart wrote:
> On Fri, Jun 05, 2020 at 03:53:15PM +0200, Jacopo Mondi wrote:
> > On Fri, Jun 05, 2020 at 03:41:24PM +0200, Eugeniu Rosca wrote:
> > > On Fri, Jun 05, 2020 at 03:29:00PM +0200, Jacopo Mondi wrote:
> > >> On Wed, May 27, 2020 at 09:15:55AM +0200, Eugeniu Rosca wrote:
> > >>> Could you kindly share the cross compilation steps for your kmsxx fork?
> > >>
> > >> I usually build it on the target :)
> > >
> > > Interesting approach. With ARM getting more and more potent, why not? :)
> > 
> > For 'small' utilities like kmsxx it's doable
> > 
> > >>> Just out of curiosity, have you ever tried to pull the display's HDMI
> > >>> cable while reading from CM2_LUT_TBL?
> > >>
> > >> Ahem, not really :) Did I get you right, you mean disconnecting the
> > >> HDMI cable from the board ?
> > >
> > > Right.
> > 
> > So, no, I have not tried. Do you see any intersting failure with the
> > mainline version ?
> 
> Jacopo, would you be able to give this a try ?

FWIW, I seem to hit pre-existing issues in vanilla rcar-du,
while unplugging HDMI cable during a cyclic suspend-resume:

HW: H3 ES2.0 Salvator-X
SW: renesas-drivers-2020-06-02-v5.7
.config: renesas_defconfig +CONFIG_PM_DEBUG +CONFIG_PM_ADVANCED_DEBUG
Use-case:

  --------8<---------
$ cat s2ram.sh
modprobe i2c-dev
echo 9 > /proc/sys/kernel/printk
i2cset -f -y 7 0x30 0x20 0x0F
echo 0 > /sys/module/suspend/parameters/pm_test_delay
echo core  > /sys/power/pm_test
echo deep > /sys/power/mem_sleep
echo 1 > /sys/power/pm_debug_messages
echo 0 > /sys/power/pm_print_times
echo mem > /sys/power/state

$ while true; do sh s2ram.sh ; done
$ # unplug HDMI cable several times

[   55.568051] PM: noirq resume of devices complete after 3.862 msecs
[   55.583253] PM: early resume of devices complete after 8.496 msecs
[   65.757023] [drm:drm_atomic_helper_wait_for_flip_done] *ERROR* [CRTC:74:crtc-1] flip_done timed out
[   75.996123] [drm:drm_atomic_helper_wait_for_flip_done] *ERROR* [CRTC:74:crtc-1] flip_done timed out
[   86.236112] [drm:drm_atomic_helper_wait_for_dependencies] *ERROR* [CRTC:74:crtc-1] flip_done timed out
[   96.476111] [drm:drm_atomic_helper_wait_for_dependencies] *ERROR* [CONNECTOR:80:HDMI-A-1] flip_done timed out
[  106.716109] [drm:drm_atomic_helper_wait_for_dependencies] *ERROR* [PLANE:45:plane-5] flip_done timed out
[  116.956111] [drm:drm_atomic_helper_wait_for_flip_done] *ERROR* [CRTC:74:crtc-1] flip_done timed out
[  127.196112] [drm:drm_atomic_helper_wait_for_dependencies] *ERROR* [CRTC:74:crtc-1] flip_done timed out
[  137.436116] [drm:drm_atomic_helper_wait_for_dependencies] *ERROR* [CONNECTOR:80:HDMI-A-1] flip_done timed out
[  147.676111] [drm:drm_atomic_helper_wait_for_dependencies] *ERROR* [PLANE:45:plane-5] flip_done timed out
[  157.916110] [drm:drm_atomic_helper_wait_for_flip_done] *ERROR* [CRTC:74:crtc-1] flip_done timed out
  --------8<---------

This looks to be unrelated to the CMM lockup I initially reported.

JYI, graphics pipelines in production R-Car3 targets are significantly
more complex (involving binding/unbinding serializer ICs at runtime
during non-trivial shutdown/suspend/resume sequences), as opposed
to the relatively straightforward VGA/HDMI outputs present on the
reference targets. So, my hope is that Renesas community can take
HDMI hot plugging seriously and make it a regular test-case during
rcar-du patch review, since this is a precondition for the R-Car3
platform and products to succeed as a whole.

BTW, if you happen to know an affordable programmable HDMI switcher
which can do the hot-plugging job in an automated test environment,
please let me know.

> 
> > >>> At least with the out-of-tree CMM implementation [*], this sends the
> > >>> R-Car3 reference targets into an unrecoverable freeze, with no lockup
> > >>> reported by the kernel (i.e. looks like an serious HW issue).
> > >>>
> > >>>> CMM functionalities are retained between suspend/resume cycles (tested with
> > >>>> suspend-to-idle) without requiring a re-programming of the LUT tables.
> > >>>
> > >>> Hmm. Is this backed up by any statement in the HW User's manual?
> > >>> This comes in contrast with the original Renesas CMM implementation [**]
> > >>> which does make use of suspend (where the freeze actually happens).
> > >>>
> > >>> Can we infer, based on your statement, that we could also get rid of
> > >>> the suspend callback in [**]?
> > >>
> > >> As Geert (thanks) explained what I've tested with is suspend-to-idle,
> > >> which retains the state of the LUT tables (and I assume other
> > >> not-yet-implemented CMM features, like CLU). I recall the out-of-tree
> > >> driver has suspend/resume routines but I never really tested that.
> > >
> > > I see. JFYI, there is a flaw in the suspend handling in the out-of-tree
> > > CMM patch [*], which renders the SoC unresponsive on HDMI hotplug. The
> > > fix is currently under review. Hopefully it will make its way to [*]
> > > in the nearest future. Just to keep in mind for the moment when CMM
> > > s2ram will become a mainline feature.
> > 
> > Thanks, let's keep this in mind. Next week I'll run a few tests again
> > with s2ram and will get back to you.
> 
> Note that the CMM driver is controlled by the DU driver. As the DU
> driver will reenable the display during resume, it will call
> rcar_du_cmm_setup() at resume time, which will reprogram the CMM. There
> should thus be no need for manual suspend/resume handling in the CMM as
> far as I can tell, but we need to ensure that the CMM is suspended
> before and resumed after the DU. I believe this could be implemented
> using device links.

Does this apply to vanilla rcar-du only (where CMM support differs
from [*]) or would also be relevant for rcar.9.6 kernel?

> 
> > >>> [*] https://github.com/renesas-rcar/du_cmm
> > >>> [**] https://github.com/renesas-rcar/du_cmm/blob/c393ed49834bdbc/meta-rcar-gen3/recipes-kernel/linux/linux-renesas/0001-drm-rcar-du-Add-DU-CMM-support.patch#L1912

Eugeniu Rosca June 9, 2020, 2:29 p.m. UTC | #14

Hi Laurent,

On Sun, Jun 07, 2020 at 05:41:58AM +0300, Laurent Pinchart wrote:
> Note that the CMM driver is controlled by the DU driver. As the DU
> driver will reenable the display during resume, it will call
> rcar_du_cmm_setup() at resume time, which will reprogram the CMM. There
> should thus be no need for manual suspend/resume handling in the CMM as
> far as I can tell, but we need to ensure that the CMM is suspended
> before and resumed after the DU. I believe this could be implemented
> using device links.

Based on below quote [*] from Jacopo's commit [**], isn't the device
link relationship already in place?

[*] Quote from commit [**]
   Enforce the probe and suspend/resume ordering of DU and CMM by
   creating a stateless device link between the two.

[**] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=8de707aeb45241
    ("drm: rcar-du: kms: Initialize CMM instances")

Jacopo Mondi June 12, 2020, 3 p.m. UTC | #15

Hi Eugeniu,

On Tue, Jun 09, 2020 at 04:29:59PM +0200, Eugeniu Rosca wrote:
> Hi Laurent,
>
> On Sun, Jun 07, 2020 at 05:41:58AM +0300, Laurent Pinchart wrote:
> > Note that the CMM driver is controlled by the DU driver. As the DU
> > driver will reenable the display during resume, it will call
> > rcar_du_cmm_setup() at resume time, which will reprogram the CMM. There
> > should thus be no need for manual suspend/resume handling in the CMM as
> > far as I can tell, but we need to ensure that the CMM is suspended
> > before and resumed after the DU. I believe this could be implemented
> > using device links.
>
> Based on below quote [*] from Jacopo's commit [**], isn't the device
> link relationship already in place?

Yes, it's in place already.

I added pm_ops to cmm just to be able to printout when suspend/resume
happens and the sequence is what comment [*] reports

[  222.909002] rcar_du_pm_suspend:505
[  223.145497] rcar_cmm_pm_suspend:193

[  223.208053] rcar_cmm_pm_resume:200
[  223.460094] rcar_du_pm_resume:513

However, Laurent mentioned that in his comment here that he expects
the opposite sequence to happen (CMM to suspend before and resume after
DU).

I still think what is implemented is correct:
- CMM is suspended after DU: when CMM is suspended, DU is not feeding
  it with data
- CMM is resumed before: once DU restart operations CMM is ready to
  receive data.

Laurent, what do you think ?

Thanks
  j

>
> [*] Quote from commit [**]
>    Enforce the probe and suspend/resume ordering of DU and CMM by
>    creating a stateless device link between the two.
>
> [**] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=8de707aeb45241
>     ("drm: rcar-du: kms: Initialize CMM instances")
>
> --
> Best regards,
> Eugeniu Rosca

Laurent Pinchart June 12, 2020, 3:10 p.m. UTC | #16

Hi Jacopo,

On Fri, Jun 12, 2020 at 05:00:32PM +0200, Jacopo Mondi wrote:
> On Tue, Jun 09, 2020 at 04:29:59PM +0200, Eugeniu Rosca wrote:
> > On Sun, Jun 07, 2020 at 05:41:58AM +0300, Laurent Pinchart wrote:
> > > Note that the CMM driver is controlled by the DU driver. As the DU
> > > driver will reenable the display during resume, it will call
> > > rcar_du_cmm_setup() at resume time, which will reprogram the CMM. There
> > > should thus be no need for manual suspend/resume handling in the CMM as
> > > far as I can tell, but we need to ensure that the CMM is suspended
> > > before and resumed after the DU. I believe this could be implemented
> > > using device links.
> >
> > Based on below quote [*] from Jacopo's commit [**], isn't the device
> > link relationship already in place?
> 
> Yes, it's in place already.
> 
> I added pm_ops to cmm just to be able to printout when suspend/resume
> happens and the sequence is what comment [*] reports
> 
> [  222.909002] rcar_du_pm_suspend:505
> [  223.145497] rcar_cmm_pm_suspend:193
> 
> [  223.208053] rcar_cmm_pm_resume:200
> [  223.460094] rcar_du_pm_resume:513
> 
> However, Laurent mentioned that in his comment here that he expects
> the opposite sequence to happen (CMM to suspend before and resume after
> DU).
> 
> I still think what is implemented is correct:
> - CMM is suspended after DU: when CMM is suspended, DU is not feeding
>   it with data
> - CMM is resumed before: once DU restart operations CMM is ready to
>   receive data.
> 
> Laurent, what do you think ?

I think I shouldn't have written the previous e-mail in the middle of
the night :-) Suspending CMM after DU is obviously correct.

> > [*] Quote from commit [**]
> >    Enforce the probe and suspend/resume ordering of DU and CMM by
> >    creating a stateless device link between the two.
> >
> > [**] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=8de707aeb45241
> >     ("drm: rcar-du: kms: Initialize CMM instances")

Jacopo Mondi June 12, 2020, 3:12 p.m. UTC | #17

Hi Eugeniu

On Mon, Jun 08, 2020 at 11:44:32AM +0200, Eugeniu Rosca wrote:
> Hello,
>
> Many thanks for your comments and involvement.
>
> On Sun, Jun 07, 2020 at 05:41:58AM +0300, Laurent Pinchart wrote:
> > On Fri, Jun 05, 2020 at 03:53:15PM +0200, Jacopo Mondi wrote:
> > > On Fri, Jun 05, 2020 at 03:41:24PM +0200, Eugeniu Rosca wrote:
> > > > On Fri, Jun 05, 2020 at 03:29:00PM +0200, Jacopo Mondi wrote:
> > > >> On Wed, May 27, 2020 at 09:15:55AM +0200, Eugeniu Rosca wrote:
> > > >>> Could you kindly share the cross compilation steps for your kmsxx fork?
> > > >>
> > > >> I usually build it on the target :)
> > > >
> > > > Interesting approach. With ARM getting more and more potent, why not? :)
> > >
> > > For 'small' utilities like kmsxx it's doable
> > >
> > > >>> Just out of curiosity, have you ever tried to pull the display's HDMI
> > > >>> cable while reading from CM2_LUT_TBL?
> > > >>
> > > >> Ahem, not really :) Did I get you right, you mean disconnecting the
> > > >> HDMI cable from the board ?
> > > >
> > > > Right.
> > >
> > > So, no, I have not tried. Do you see any intersting failure with the
> > > mainline version ?
> >
> > Jacopo, would you be able to give this a try ?
>
> FWIW, I seem to hit pre-existing issues in vanilla rcar-du,
> while unplugging HDMI cable during a cyclic suspend-resume:
>
> HW: H3 ES2.0 Salvator-X
> SW: renesas-drivers-2020-06-02-v5.7
> .config: renesas_defconfig +CONFIG_PM_DEBUG +CONFIG_PM_ADVANCED_DEBUG
> Use-case:
>
>   --------8<---------
> $ cat s2ram.sh
> modprobe i2c-dev
> echo 9 > /proc/sys/kernel/printk
> i2cset -f -y 7 0x30 0x20 0x0F

According to
https://elinux.org/R-Car/Boards/Salvator-X#Suspend-to-RAM
this is not needed anymore

> echo 0 > /sys/module/suspend/parameters/pm_test_delay
> echo core  > /sys/power/pm_test
> echo deep > /sys/power/mem_sleep
> echo 1 > /sys/power/pm_debug_messages
> echo 0 > /sys/power/pm_print_times
> echo mem > /sys/power/state
>
> $ while true; do sh s2ram.sh ; done
> $ # unplug HDMI cable several times

I tried unplugging an plugging the cable while the system was
suspended and after resume but I was not able to reproduce anything.

Could you provide more precise instructions on how to reproduce this ?
I.e. when to disconnect the cable to trigger the below error.

Thanks
  j

>
> [   55.568051] PM: noirq resume of devices complete after 3.862 msecs
> [   55.583253] PM: early resume of devices complete after 8.496 msecs
> [   65.757023] [drm:drm_atomic_helper_wait_for_flip_done] *ERROR* [CRTC:74:crtc-1] flip_done timed out
> [   75.996123] [drm:drm_atomic_helper_wait_for_flip_done] *ERROR* [CRTC:74:crtc-1] flip_done timed out
> [   86.236112] [drm:drm_atomic_helper_wait_for_dependencies] *ERROR* [CRTC:74:crtc-1] flip_done timed out
> [   96.476111] [drm:drm_atomic_helper_wait_for_dependencies] *ERROR* [CONNECTOR:80:HDMI-A-1] flip_done timed out
> [  106.716109] [drm:drm_atomic_helper_wait_for_dependencies] *ERROR* [PLANE:45:plane-5] flip_done timed out
> [  116.956111] [drm:drm_atomic_helper_wait_for_flip_done] *ERROR* [CRTC:74:crtc-1] flip_done timed out
> [  127.196112] [drm:drm_atomic_helper_wait_for_dependencies] *ERROR* [CRTC:74:crtc-1] flip_done timed out
> [  137.436116] [drm:drm_atomic_helper_wait_for_dependencies] *ERROR* [CONNECTOR:80:HDMI-A-1] flip_done timed out
> [  147.676111] [drm:drm_atomic_helper_wait_for_dependencies] *ERROR* [PLANE:45:plane-5] flip_done timed out
> [  157.916110] [drm:drm_atomic_helper_wait_for_flip_done] *ERROR* [CRTC:74:crtc-1] flip_done timed out
>   --------8<---------
>
> This looks to be unrelated to the CMM lockup I initially reported.
>
> JYI, graphics pipelines in production R-Car3 targets are significantly
> more complex (involving binding/unbinding serializer ICs at runtime
> during non-trivial shutdown/suspend/resume sequences), as opposed
> to the relatively straightforward VGA/HDMI outputs present on the
> reference targets. So, my hope is that Renesas community can take
> HDMI hot plugging seriously and make it a regular test-case during
> rcar-du patch review, since this is a precondition for the R-Car3
> platform and products to succeed as a whole.
>
> BTW, if you happen to know an affordable programmable HDMI switcher
> which can do the hot-plugging job in an automated test environment,
> please let me know.
>
> >
> > > >>> At least with the out-of-tree CMM implementation [*], this sends the
> > > >>> R-Car3 reference targets into an unrecoverable freeze, with no lockup
> > > >>> reported by the kernel (i.e. looks like an serious HW issue).
> > > >>>
> > > >>>> CMM functionalities are retained between suspend/resume cycles (tested with
> > > >>>> suspend-to-idle) without requiring a re-programming of the LUT tables.
> > > >>>
> > > >>> Hmm. Is this backed up by any statement in the HW User's manual?
> > > >>> This comes in contrast with the original Renesas CMM implementation [**]
> > > >>> which does make use of suspend (where the freeze actually happens).
> > > >>>
> > > >>> Can we infer, based on your statement, that we could also get rid of
> > > >>> the suspend callback in [**]?
> > > >>
> > > >> As Geert (thanks) explained what I've tested with is suspend-to-idle,
> > > >> which retains the state of the LUT tables (and I assume other
> > > >> not-yet-implemented CMM features, like CLU). I recall the out-of-tree
> > > >> driver has suspend/resume routines but I never really tested that.
> > > >
> > > > I see. JFYI, there is a flaw in the suspend handling in the out-of-tree
> > > > CMM patch [*], which renders the SoC unresponsive on HDMI hotplug. The
> > > > fix is currently under review. Hopefully it will make its way to [*]
> > > > in the nearest future. Just to keep in mind for the moment when CMM
> > > > s2ram will become a mainline feature.
> > >
> > > Thanks, let's keep this in mind. Next week I'll run a few tests again
> > > with s2ram and will get back to you.
> >
> > Note that the CMM driver is controlled by the DU driver. As the DU
> > driver will reenable the display during resume, it will call
> > rcar_du_cmm_setup() at resume time, which will reprogram the CMM. There
> > should thus be no need for manual suspend/resume handling in the CMM as
> > far as I can tell, but we need to ensure that the CMM is suspended
> > before and resumed after the DU. I believe this could be implemented
> > using device links.
>
> Does this apply to vanilla rcar-du only (where CMM support differs
> from [*]) or would also be relevant for rcar.9.6 kernel?
>
> >
> > > >>> [*] https://github.com/renesas-rcar/du_cmm
> > > >>> [**] https://github.com/renesas-rcar/du_cmm/blob/c393ed49834bdbc/meta-rcar-gen3/recipes-kernel/linux/linux-renesas/0001-drm-rcar-du-Add-DU-CMM-support.patch#L1912
>
> --
> Best regards,
> Eugeniu Rosca

Eugeniu Rosca June 12, 2020, 3:36 p.m. UTC | #18

Hi,

On Fri, Jun 12, 2020 at 06:10:05PM +0300, Laurent Pinchart wrote:
> On Fri, Jun 12, 2020 at 05:00:32PM +0200, Jacopo Mondi wrote:
> > On Tue, Jun 09, 2020 at 04:29:59PM +0200, Eugeniu Rosca wrote:
> > > On Sun, Jun 07, 2020 at 05:41:58AM +0300, Laurent Pinchart wrote:
> > > > Note that the CMM driver is controlled by the DU driver. As the DU
> > > > driver will reenable the display during resume, it will call
> > > > rcar_du_cmm_setup() at resume time, which will reprogram the CMM. There
> > > > should thus be no need for manual suspend/resume handling in the CMM as
> > > > far as I can tell, but we need to ensure that the CMM is suspended
> > > > before and resumed after the DU. I believe this could be implemented
> > > > using device links.
> > >
> > > Based on below quote [*] from Jacopo's commit [**], isn't the device
> > > link relationship already in place?
> > 
> > Yes, it's in place already.
> > 
> > I added pm_ops to cmm just to be able to printout when suspend/resume
> > happens and the sequence is what comment [*] reports
> > 
> > [  222.909002] rcar_du_pm_suspend:505
> > [  223.145497] rcar_cmm_pm_suspend:193
> > 
> > [  223.208053] rcar_cmm_pm_resume:200
> > [  223.460094] rcar_du_pm_resume:513
> > 
> > However, Laurent mentioned that in his comment here that he expects
> > the opposite sequence to happen (CMM to suspend before and resume after
> > DU).
> > 
> > I still think what is implemented is correct:
> > - CMM is suspended after DU: when CMM is suspended, DU is not feeding
> >   it with data
> > - CMM is resumed before: once DU restart operations CMM is ready to
> >   receive data.
> > 
> > Laurent, what do you think ?
> 
> I think I shouldn't have written the previous e-mail in the middle of
> the night :-) Suspending CMM after DU is obviously correct.

Thanks to Renesas team (kudos to Gotthard and Michael), we've
figured out that below sequence of clock handling (happening during
concurrent suspend and HDMI display unplug) leads to SoC lockup:

cmm1 OFF 	(caused by HDMI unplug)
x21-clock OFF 	(caused by HDMI unplug)
du1 OFF 	(caused by HDMI unplug)
cmm1 ON (caused by suspend to ram, as preparation for CMM register save)
# Freeze happens

That seems to be explained by Chapter 35A.4.3 "Restriction of enabling
clock signal of the CMM" of HW User's manual (Rev.2.00 Jul 2019):

 -----8<-----
 When the clock signal of the CMM is enabled (RMSTPCR7.CMMn or
 SMSTPCR7.CMMn = 0), the clock signal of the DU should be also enabled
 (RMSTPCR7.DUn or SMSTPCR7.DUn = 0).
 -----8<-----

So, the lesson learned from the above is: do not enable the CMMi clock
while the DUi clock is disabled. I expect this to also potentially
give some input w.r.t. what to suspend/resume first, CMM or DU.

> 
> > > [*] Quote from commit [**]
> > >    Enforce the probe and suspend/resume ordering of DU and CMM by
> > >    creating a stateless device link between the two.
> > >
> > > [**] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=8de707aeb45241
> > >     ("drm: rcar-du: kms: Initialize CMM instances")

Laurent Pinchart June 12, 2020, 3:50 p.m. UTC | #19

Hi Eugeniu,

On Fri, Jun 12, 2020 at 05:36:07PM +0200, Eugeniu Rosca wrote:
> On Fri, Jun 12, 2020 at 06:10:05PM +0300, Laurent Pinchart wrote:
> > On Fri, Jun 12, 2020 at 05:00:32PM +0200, Jacopo Mondi wrote:
> > > On Tue, Jun 09, 2020 at 04:29:59PM +0200, Eugeniu Rosca wrote:
> > > > On Sun, Jun 07, 2020 at 05:41:58AM +0300, Laurent Pinchart wrote:
> > > > > Note that the CMM driver is controlled by the DU driver. As the DU
> > > > > driver will reenable the display during resume, it will call
> > > > > rcar_du_cmm_setup() at resume time, which will reprogram the CMM. There
> > > > > should thus be no need for manual suspend/resume handling in the CMM as
> > > > > far as I can tell, but we need to ensure that the CMM is suspended
> > > > > before and resumed after the DU. I believe this could be implemented
> > > > > using device links.
> > > >
> > > > Based on below quote [*] from Jacopo's commit [**], isn't the device
> > > > link relationship already in place?
> > > 
> > > Yes, it's in place already.
> > > 
> > > I added pm_ops to cmm just to be able to printout when suspend/resume
> > > happens and the sequence is what comment [*] reports
> > > 
> > > [  222.909002] rcar_du_pm_suspend:505
> > > [  223.145497] rcar_cmm_pm_suspend:193
> > > 
> > > [  223.208053] rcar_cmm_pm_resume:200
> > > [  223.460094] rcar_du_pm_resume:513
> > > 
> > > However, Laurent mentioned that in his comment here that he expects
> > > the opposite sequence to happen (CMM to suspend before and resume after
> > > DU).
> > > 
> > > I still think what is implemented is correct:
> > > - CMM is suspended after DU: when CMM is suspended, DU is not feeding
> > >   it with data
> > > - CMM is resumed before: once DU restart operations CMM is ready to
> > >   receive data.
> > > 
> > > Laurent, what do you think ?
> > 
> > I think I shouldn't have written the previous e-mail in the middle of
> > the night :-) Suspending CMM after DU is obviously correct.
> 
> Thanks to Renesas team (kudos to Gotthard and Michael), we've
> figured out that below sequence of clock handling (happening during
> concurrent suspend and HDMI display unplug) leads to SoC lockup:
> 
> cmm1 OFF 	(caused by HDMI unplug)
> x21-clock OFF 	(caused by HDMI unplug)
> du1 OFF 	(caused by HDMI unplug)
> cmm1 ON (caused by suspend to ram, as preparation for CMM register save)
> # Freeze happens
> 
> That seems to be explained by Chapter 35A.4.3 "Restriction of enabling
> clock signal of the CMM" of HW User's manual (Rev.2.00 Jul 2019):
> 
>  -----8<-----
>  When the clock signal of the CMM is enabled (RMSTPCR7.CMMn or
>  SMSTPCR7.CMMn = 0), the clock signal of the DU should be also enabled
>  (RMSTPCR7.DUn or SMSTPCR7.DUn = 0).
>  -----8<-----
> 
> So, the lesson learned from the above is: do not enable the CMMi clock
> while the DUi clock is disabled. I expect this to also potentially
> give some input w.r.t. what to suspend/resume first, CMM or DU.

This may be an ugly one. The DU driver needs to disable the CMM when
suspending, and enabling the CMM when resuming. To do so, it calls
functions of the CMM driver, and those functions access CMM registers.
We can't do so while the CMM is suspended, so the DU has to suspend
before the CMM, and resume after the CMM.

On the other hand, as you state here, we need to make sure the CMM clock
is disabled first. The CMM thus needs to suspend before the DU, and
resume after the DU.

Those are conflicting requirements.

One option could be to use the .suspend_late() and .resume_early() PM
operations for the DU, to turn the DU clock off late and turn it back on
early. Integrating it with the DRM suspend/resume helpers will likely be
complicated though. I wonder if we could find a more elegant solution.

I the above sequence, you list "cmm1 ON" as a preparation for CMM
register save. We don't need to save any register, the CMM driver has no
.suspend() handler. The PM core should really skip waking up the device
at that point (I recall complaining about the spurious wake ups at
suspend time a while ago, but that neevr got addressed). Geert, any
opinion on that ?

> > > > [*] Quote from commit [**]
> > > >    Enforce the probe and suspend/resume ordering of DU and CMM by
> > > >    creating a stateless device link between the two.
> > > >
> > > > [**] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=8de707aeb45241
> > > >     ("drm: rcar-du: kms: Initialize CMM instances")

Eugeniu Rosca June 15, 2020, 2:17 p.m. UTC | #20

Hi Jacopo,

On Fri, Jun 12, 2020 at 05:12:09PM +0200, Jacopo Mondi wrote:
> On Mon, Jun 08, 2020 at 11:44:32AM +0200, Eugeniu Rosca wrote:
> > FWIW, I seem to hit pre-existing issues in vanilla rcar-du,
> > while unplugging HDMI cable during a cyclic suspend-resume:
> >
> > HW: H3 ES2.0 Salvator-X
> > SW: renesas-drivers-2020-06-02-v5.7
> > .config: renesas_defconfig +CONFIG_PM_DEBUG +CONFIG_PM_ADVANCED_DEBUG
> > Use-case:
> >
> >   --------8<---------
> > $ cat s2ram.sh
> > modprobe i2c-dev
> > echo 9 > /proc/sys/kernel/printk
> > i2cset -f -y 7 0x30 0x20 0x0F
> 
> According to
> https://elinux.org/R-Car/Boards/Salvator-X#Suspend-to-RAM
> this is not needed anymore

Good to know. Thanks for the useful remark.

> > echo 0 > /sys/module/suspend/parameters/pm_test_delay
> > echo core  > /sys/power/pm_test
> > echo deep > /sys/power/mem_sleep
> > echo 1 > /sys/power/pm_debug_messages
> > echo 0 > /sys/power/pm_print_times
> > echo mem > /sys/power/state
> >
> > $ while true; do sh s2ram.sh ; done
> > $ # unplug HDMI cable several times
> 
> I tried unplugging an plugging the cable while the system was
> suspended and after resume but I was not able to reproduce anything.

Your comment sounds like you suspended the system once and resumed it
afterwards, while my description mentions "cyclic" :), meaning:

$ while true; do sh s2ram.sh; done
$ # connect-disconnect the hdmi display a couple of times
$ # NOTE: to avoid this manual step, I am thinking of a USB-controlled
    HDMI switcher long-term

> 
> Could you provide more precise instructions on how to reproduce this ?
> I.e. when to disconnect the cable to trigger the below error.

See above :)

BTW, using renesas-drivers-2020-06-02-v5.7 as base and performing the
use-case just described, I got today (with minimal effort):

[  459.321733] Enabling non-boot CPUs ...
[  459.331132] Detected PIPT I-cache on CPU1
[  459.331189] CPU1: Booted secondary processor 0x0000000001 [0x411fd073]
[  459.332312] CPU1 is up
[  459.345635] Detected PIPT I-cache on CPU2
[  459.345671] CPU2: Booted secondary processor 0x0000000002 [0x411fd073]
[  459.346624] CPU2 is up
[  459.359912] Detected PIPT I-cache on CPU3
[  459.359942] CPU3: Booted secondary processor 0x0000000003 [0x411fd073]
[  459.360918] CPU3 is up
[  459.374183] Detected VIPT I-cache on CPU4
[  459.374252] CPU4: Booted secondary processor 0x0000000100 [0x410fd034]
[  459.375875] cpufreq: cpufreq_online: CPU4: Running at unlisted freq: 1199999 KHz
[  459.394204] cpufreq: cpufreq_online: CPU4: Unlisted initial frequency changed to: 1200000 KHz
[  459.403879] CPU4 is up
[  459.406469] Detected VIPT I-cache on CPU5
[  459.406519] CPU5: Booted secondary processor 0x0000000101 [0x410fd034]
[  459.408520] CPU5 is up
[  459.421762] Detected VIPT I-cache on CPU6
[  459.421810] CPU6: Booted secondary processor 0x0000000102 [0x410fd034]
[  459.423831] CPU6 is up
[  459.437114] Detected VIPT I-cache on CPU7
[  459.437164] CPU7: Booted secondary processor 0x0000000103 [0x410fd034]
[  459.439258] CPU7 is up
[  459.456217] PM: noirq resume of devices complete after 3.878 msecs
[  459.471529] PM: early resume of devices complete after 8.590 msecs
[  469.726906] [drm:drm_atomic_helper_wait_for_dependencies] *ERROR* [CRTC:76:crtc-3] flip_done timed out
[  479.966889] [drm:drm_atomic_helper_wait_for_dependencies] *ERROR* [CONNECTOR:78:VGA-1] flip_done timed out
[  490.206887] [drm:drm_atomic_helper_wait_for_dependencies] *ERROR* [PLANE:34:plane-1] flip_done timed out
[  490.282873] rcar-du feb00000.display: page flip timeout
[  490.288431] ------------[ cut here ]------------
[  490.293092] WARNING: CPU: 0 PID: 123 at drivers/gpu/drm/rcar-du/rcar_du_crtc.c:787 rcar_du_crtc_atomic_begin+0x64/0x70
[  490.303854] Modules linked in:
[  490.306933] CPU: 0 PID: 123 Comm: irq/192-fead000 Not tainted 5.7.0-arm64-renesas-00012-gbc5411732961 #131
[  490.316647] Hardware name: Renesas Salvator-X board based on r8a77951 (DT)
[  490.323568] pstate: 20000005 (nzCv daif -PAN -UAO BTYPE=--)
[  490.329176] pc : rcar_du_crtc_atomic_begin+0x64/0x70
[  490.334174] lr : drm_atomic_helper_commit_planes+0x70/0x220
[  490.339781] sp : ffff80001258b960
[  490.343115] x29: ffff80001258b960 x28: 0000000000000038 
[  490.348463] x27: ffff0006f676a800 x26: ffff8000111c3718 
[  490.353811] x25: ffff0006f5e574c0 x24: ffff0006f6728000 
[  490.359157] x23: 0000000000000038 x22: 0000000000000000 
[  490.364503] x21: 0000000000000001 x20: 0000000000000002 
[  490.369849] x19: ffff0006f5e58e08 x18: ffffffffffffffff 
[  490.375195] x17: 0000000000000000 x16: 0000000000000000 
[  490.380541] x15: ffff8000111c09c8 x14: 0000000000000000 
[  490.385887] x13: 0000000000000000 x12: 0000000000000000 
[  490.391233] x11: 0000000000000000 x10: 0000000000000000 
[  490.396579] x9 : 0000000000000000 x8 : 0000000000000000 
[  490.401924] x7 : 0000000000000000 x6 : ffff0006f5e5aab8 
[  490.407270] x5 : 0000000000003a8b x4 : ffff0006f3154438 
[  490.412616] x3 : ffff0006f6728000 x2 : ffff800010649338 
[  490.417962] x1 : ffff0006f228dc00 x0 : 0000000000000000 
[  490.423308] Call trace:
[  490.425771]  rcar_du_crtc_atomic_begin+0x64/0x70
[  490.430418]  drm_atomic_helper_commit_planes+0x70/0x220
[  490.435678]  rcar_du_atomic_commit_tail+0xa0/0xd8
[  490.440412]  commit_tail+0x9c/0x160
[  490.443924]  drm_atomic_helper_commit+0x178/0x190
[  490.448661]  drm_atomic_commit+0x48/0x58
[  490.452611]  drm_client_modeset_commit_atomic+0x18c/0x280
[  490.458045]  drm_client_modeset_commit_locked+0x50/0x1e0
[  490.463390]  drm_client_modeset_commit+0x2c/0x50
[  490.468040]  drm_fb_helper_restore_fbdev_mode_unlocked+0x74/0xe8
[  490.474085]  drm_fb_helper_set_par+0x2c/0x60
[  490.478383]  drm_fb_helper_hotplug_event.part.20+0xc0/0xd0
[  490.483904]  drm_fbdev_client_hotplug+0xc8/0x1d0
[  490.488551]  drm_client_dev_hotplug+0x78/0xc0
[  490.492939]  drm_kms_helper_hotplug_event+0x30/0x40
[  490.497848]  drm_helper_hpd_irq_event+0x12c/0x160
[  490.502583]  dw_hdmi_irq+0x118/0x1e0
[  490.506185]  irq_thread_fn+0x28/0x88
[  490.509783]  irq_thread+0x13c/0x1d8
[  490.513298]  kthread+0x150/0x158
[  490.516549]  ret_from_fork+0x10/0x18
[  490.520147] irq event stamp: 12676
[  490.523576] hardirqs last  enabled at (12675): [<ffff800010a725cc>] _raw_spin_unlock_irqrestore+0x7c/0x90
[  490.533206] hardirqs last disabled at (12676): [<ffff800010024918>] do_debug_exception+0x1a0/0x234
[  490.542223] softirqs last  enabled at (12634): [<ffff800010001598>] efi_header_end+0x598/0x5ac
[  490.550892] softirqs last disabled at (12625): [<ffff80001007cf9c>] irq_exit+0x13c/0x148
[  490.559032] ---[ end trace 5cdf3cb912c7ed2d ]---
[  490.563942] vsp1 fea28000.vsp: vsp1_du_pipeline_setup_inputs: failed to setup BRU source
[  490.572213] vsp1 fea28000.vsp: vsp1_du_pipeline_setup_inputs: failed to setup BRU source
[  490.580410] ------------[ cut here ]------------
[  490.585087] refcount_t: addition on 0; use-after-free.
[  490.590316] WARNING: CPU: 0 PID: 123 at lib/refcount.c:25 refcount_warn_saturate+0x120/0x140
[  490.598807] Modules linked in:
[  490.601883] CPU: 0 PID: 123 Comm: irq/192-fead000 Tainted: G        W         5.7.0-arm64-renesas-00012-gbc5411732961 #131
[  490.612994] Hardware name: Renesas Salvator-X board based on r8a77951 (DT)
[  490.619912] pstate: 40000005 (nZcv daif -PAN -UAO BTYPE=--)
[  490.625521] pc : refcount_warn_saturate+0x120/0x140
[  490.630430] lr : refcount_warn_saturate+0x120/0x140
[  490.635337] sp : ffff80001258b960
[  490.638671] x29: ffff80001258b960 x28: 0000000000000038 
[  490.644018] x27: ffff0006f676a800 x26: ffff8000111c3718 
[  490.649364] x25: ffff0006f228c200 x24: ffff0006f228c400 
[  490.654711] x23: 0000000000000038 x22: 0000000000000001 
[  490.660057] x21: ffff0006f3154000 x20: 0000000000000000 
[  490.665403] x19: ffff0006f228ec00 x18: ffffffffffffffff 
[  490.670748] x17: 0000000000000000 x16: 000000007a11fc18 
[  490.676095] x15: ffff8000111c09c8 x14: ffff80009258b677 
[  490.681441] x13: ffff80001258b685 x12: ffff8000111e2000 
[  490.686786] x11: 0000000005f5e0ff x10: 00000000000000b8 
[  490.692132] x9 : 0000000000000000 x8 : 0000000000000000 
[  490.697478] x7 : 0000000000000001 x6 : 0000000000000000 
[  490.702824] x5 : 0000000000000000 x4 : 0000000000000000 
[  490.708170] x3 : 0000000000000000 x2 : ffff0006fa12def8 
[  490.713516] x1 : f874420ef852bd00 x0 : 0000000000000000 
[  490.718862] Call trace:
[  490.721325]  refcount_warn_saturate+0x120/0x140
[  490.725884]  drm_atomic_helper_commit_hw_done+0x140/0x150
[  490.731318]  rcar_du_atomic_commit_tail+0xb4/0xd8
[  490.736051]  commit_tail+0x9c/0x160
[  490.739561]  drm_atomic_helper_commit+0x178/0x190
[  490.744296]  drm_atomic_commit+0x48/0x58
[  490.748244]  drm_client_modeset_commit_atomic+0x18c/0x280
[  490.753678]  drm_client_modeset_commit_locked+0x50/0x1e0
[  490.759023]  drm_client_modeset_commit+0x2c/0x50
[  490.763671]  drm_fb_helper_restore_fbdev_mode_unlocked+0x74/0xe8
[  490.769717]  drm_fb_helper_set_par+0x2c/0x60
[  490.774014]  drm_fb_helper_hotplug_event.part.20+0xc0/0xd0
[  490.779535]  drm_fbdev_client_hotplug+0xc8/0x1d0
[  490.784182]  drm_client_dev_hotplug+0x78/0xc0
[  490.788567]  drm_kms_helper_hotplug_event+0x30/0x40
[  490.793476]  drm_helper_hpd_irq_event+0x12c/0x160
[  490.798210]  dw_hdmi_irq+0x118/0x1e0
[  490.801809]  irq_thread_fn+0x28/0x88
[  490.805408]  irq_thread+0x13c/0x1d8
[  490.808919]  kthread+0x150/0x158
[  490.812167]  ret_from_fork+0x10/0x18
[  490.815764] irq event stamp: 12796
[  490.819189] hardirqs last  enabled at (12795): [<ffff8000100eecf0>] console_unlock+0x418/0x638
[  490.827855] hardirqs last disabled at (12796): [<ffff800010024918>] do_debug_exception+0x1a0/0x234
[  490.836871] softirqs last  enabled at (12792): [<ffff800010001598>] efi_header_end+0x598/0x5ac
[  490.845538] softirqs last disabled at (12785): [<ffff80001007cf9c>] irq_exit+0x13c/0x148
[  490.853678] ---[ end trace 5cdf3cb912c7ed2e ]---
[  490.858414] ------------[ cut here ]------------
[  490.863065] WARNING: CPU: 0 PID: 123 at drivers/gpu/drm/drm_atomic_helper.c:2312 drm_atomic_helper_commit_hw_done+0x130/0x150
[  490.874438] Modules linked in:
[  490.877513] CPU: 0 PID: 123 Comm: irq/192-fead000 Tainted: G        W         5.7.0-arm64-renesas-00012-gbc5411732961 #131
[  490.888624] Hardware name: Renesas Salvator-X board based on r8a77951 (DT)
[  490.895542] pstate: 00000005 (nzcv daif -PAN -UAO BTYPE=--)
[  490.901150] pc : drm_atomic_helper_commit_hw_done+0x130/0x150
[  490.906932] lr : drm_atomic_helper_commit_hw_done+0x118/0x150
[  490.912713] sp : ffff80001258b970
[  490.916047] x29: ffff80001258b970 x28: 0000000000000038 
[  490.921393] x27: ffff0006f676a800 x26: ffff8000111c3718 
[  490.926739] x25: ffff0006f228dc00 x24: ffff0006f228f000 
[  490.932085] x23: 0000000000000038 x22: 0000000000000001 
[  490.937431] x21: ffff0006f3154000 x20: 0000000000000001 
[  490.942777] x19: ffff0006f228cc00 x18: ffffffffffffffff 
[  490.948122] x17: 0000000000000000 x16: 000000007a11fc18 
[  490.953468] x15: ffff8000111c09c8 x14: ffff80009258b677 
[  490.958814] x13: ffff80001258b685 x12: ffff8000111e2000 
[  490.964160] x11: 0000000005f5e0ff x10: 00000000000000b8 
[  490.969506] x9 : 0000000000000080 x8 : 0000000000000000 
[  490.974851] x7 : 0000000000000001 x6 : 0000000000000000 
[  490.980196] x5 : 0000000000000000 x4 : 0000000000006d40 
[  490.985542] x3 : 0000000000000000 x2 : 0000000000000002 
[  490.990887] x1 : 0000000000000001 x0 : ffff0006f5a76580 
[  490.996232] Call trace:
[  490.998694]  drm_atomic_helper_commit_hw_done+0x130/0x150
[  491.004127]  rcar_du_atomic_commit_tail+0xb4/0xd8
[  491.008861]  commit_tail+0x9c/0x160
[  491.012371]  drm_atomic_helper_commit+0x178/0x190
[  491.017105]  drm_atomic_commit+0x48/0x58
[  491.021053]  drm_client_modeset_commit_atomic+0x18c/0x280
[  491.026486]  drm_client_modeset_commit_locked+0x50/0x1e0
[  491.031832]  drm_client_modeset_commit+0x2c/0x50
[  491.036480]  drm_fb_helper_restore_fbdev_mode_unlocked+0x74/0xe8
[  491.042524]  drm_fb_helper_set_par+0x2c/0x60
[  491.046822]  drm_fb_helper_hotplug_event.part.20+0xc0/0xd0
[  491.052343]  drm_fbdev_client_hotplug+0xc8/0x1d0
[  491.056989]  drm_client_dev_hotplug+0x78/0xc0
[  491.061374]  drm_kms_helper_hotplug_event+0x30/0x40
[  491.066283]  drm_helper_hpd_irq_event+0x12c/0x160
[  491.071017]  dw_hdmi_irq+0x118/0x1e0
[  491.074615]  irq_thread_fn+0x28/0x88
[  491.078214]  irq_thread+0x13c/0x1d8
[  491.081724]  kthread+0x150/0x158
[  491.084973]  ret_from_fork+0x10/0x18
[  491.088569] irq event stamp: 12824
[  491.091996] hardirqs last  enabled at (12823): [<ffff800010246bd0>] kfree+0x308/0x430
[  491.099877] hardirqs last disabled at (12824): [<ffff800010024918>] do_debug_exception+0x1a0/0x234
[  491.108892] softirqs last  enabled at (12818): [<ffff800010001598>] efi_header_end+0x598/0x5ac
[  491.117558] softirqs last disabled at (12799): [<ffff80001007cf9c>] irq_exit+0x13c/0x148
[  491.125699] ---[ end trace 5cdf3cb912c7ed2f ]---
[  491.130416] ------------[ cut here ]------------
[  491.135067] WARNING: CPU: 0 PID: 123 at drivers/gpu/drm/drm_atomic_helper.c:2312 drm_atomic_helper_commit_hw_done+0x130/0x150
[  491.146440] Modules linked in:
[  491.149515] CPU: 0 PID: 123 Comm: irq/192-fead000 Tainted: G        W         5.7.0-arm64-renesas-00012-gbc5411732961 #131
[  491.160625] Hardware name: Renesas Salvator-X board based on r8a77951 (DT)
[  491.167543] pstate: 00000005 (nzcv daif -PAN -UAO BTYPE=--)
[  491.173150] pc : drm_atomic_helper_commit_hw_done+0x130/0x150
[  491.178932] lr : drm_atomic_helper_commit_hw_done+0xc8/0x150
[  491.184625] sp : ffff80001258b970
[  491.187960] x29: ffff80001258b970 x28: 0000000000000038 
[  491.193306] x27: ffff0006f676a800 x26: ffff8000111c3718 
[  491.198651] x25: ffff0006f228d000 x24: ffff0006f228f600 
[  491.203998] x23: 0000000000000038 x22: 0000000000000001 
[  491.209343] x21: ffff0006f3154000 x20: 0000000000000003 
[  491.214689] x19: ffff0006f228fa00 x18: ffffffffffffffff 
[  491.220035] x17: 0000000000000000 x16: 000000007a11fc18 
[  491.225381] x15: ffff8000111c09c8 x14: ffff80009258b677 
[  491.230727] x13: ffff80001258b685 x12: ffff8000111e2000 
[  491.236073] x11: 0000000005f5e0ff x10: 00000000000000b8 
[  491.241418] x9 : 0000000000000080 x8 : 0000000000000000 
[  491.246764] x7 : 0000000000000001 x6 : 0000000000000000 
[  491.252109] x5 : 0000000000000001 x4 : 0000000000000000 
[  491.257455] x3 : 0000000000000000 x2 : 0000000000000002 
[  491.262801] x1 : 0000000000000001 x0 : ffff0006f5a76c80 
[  491.268146] Call trace:
[  491.270607]  drm_atomic_helper_commit_hw_done+0x130/0x150
[  491.276040]  rcar_du_atomic_commit_tail+0xb4/0xd8
[  491.280774]  commit_tail+0x9c/0x160
[  491.284284]  drm_atomic_helper_commit+0x178/0x190
[  491.289018]  drm_atomic_commit+0x48/0x58
[  491.292966]  drm_client_modeset_commit_atomic+0x18c/0x280
[  491.298399]  drm_client_modeset_commit_locked+0x50/0x1e0
[  491.303745]  drm_client_modeset_commit+0x2c/0x50
[  491.308392]  drm_fb_helper_restore_fbdev_mode_unlocked+0x74/0xe8
[  491.314437]  drm_fb_helper_set_par+0x2c/0x60
[  491.318734]  drm_fb_helper_hotplug_event.part.20+0xc0/0xd0
[  491.324254]  drm_fbdev_client_hotplug+0xc8/0x1d0
[  491.328901]  drm_client_dev_hotplug+0x78/0xc0
[  491.333286]  drm_kms_helper_hotplug_event+0x30/0x40
[  491.338194]  drm_helper_hpd_irq_event+0x12c/0x160
[  491.342927]  dw_hdmi_irq+0x118/0x1e0
[  491.346526]  irq_thread_fn+0x28/0x88
[  491.350124]  irq_thread+0x13c/0x1d8
[  491.353635]  kthread+0x150/0x158
[  491.356884]  ret_from_fork+0x10/0x18
[  491.360480] irq event stamp: 12850
[  491.363904] hardirqs last  enabled at (12849): [<ffff800010a725cc>] _raw_spin_unlock_irqrestore+0x7c/0x90
[  491.373532] hardirqs last disabled at (12850): [<ffff800010024918>] do_debug_exception+0x1a0/0x234
[  491.382548] softirqs last  enabled at (12842): [<ffff800010001598>] efi_header_end+0x598/0x5ac
[  491.391213] softirqs last disabled at (12827): [<ffff80001007cf9c>] irq_exit+0x13c/0x148
[  491.399354] ---[ end trace 5cdf3cb912c7ed30 ]---
[  491.404400] sched: RT throttling activated
[  501.470891] [drm:drm_atomic_helper_wait_for_flip_done] *ERROR* [CRTC:74:crtc-1] flip_done timed out
[  511.710886] [drm:drm_atomic_helper_wait_for_flip_done] *ERROR* [CRTC:76:crtc-3] flip_done timed out
[  511.720062] ------------[ cut here ]------------
[  511.724740] refcount_t: underflow; use-after-free.
[  511.729624] WARNING: CPU: 0 PID: 123 at lib/refcount.c:28 refcount_warn_saturate+0xcc/0x140
[  511.738027] Modules linked in:
[  511.741102] CPU: 0 PID: 123 Comm: irq/192-fead000 Tainted: G        W         5.7.0-arm64-renesas-00012-gbc5411732961 #131
[  511.752214] Hardware name: Renesas Salvator-X board based on r8a77951 (DT)
[  511.759132] pstate: 40000005 (nZcv daif -PAN -UAO BTYPE=--)
[  511.764740] pc : refcount_warn_saturate+0xcc/0x140
[  511.769561] lr : refcount_warn_saturate+0xcc/0x140
[  511.774381] sp : ffff80001258b9a0
[  511.777715] x29: ffff80001258b9a0 x28: 0000000000000038 
[  511.783062] x27: ffff0006f676a800 x26: ffff8000111c3718 
[  511.788408] x25: ffff0006f5e574c0 x24: ffff0006f6728440 
[  511.793755] x23: 0000000000000038 x22: 0000000000000001 
[  511.799101] x21: 0000000000000000 x20: 0000000000000000 
[  511.804447] x19: ffff0006f228c200 x18: ffffffffffffffff 
[  511.809793] x17: 0000000000000000 x16: 000000007a11fc18 
[  511.815139] x15: ffff8000111c09c8 x14: ffff80009258b6b7 
[  511.820485] x13: ffff80001258b6c5 x12: ffff8000111e2000 
[  511.825830] x11: 0000000005f5e0ff x10: 00000000000000b8 
[  511.831176] x9 : 0000000000000000 x8 : 0000000000000000 
[  511.836522] x7 : 0000000000000001 x6 : 0000000000000000 
[  511.841867] x5 : 0000000000000000 x4 : 0000000000000000 
[  511.847212] x3 : 0000000000000000 x2 : ffff0006fa12def8 
[  511.852558] x1 : f874420ef852bd00 x0 : 0000000000000000 
[  511.857904] Call trace:
[  511.860367]  refcount_warn_saturate+0xcc/0x140
[  511.864841]  __drm_atomic_helper_crtc_destroy_state+0xdc/0x100
[  511.870711]  rcar_du_crtc_atomic_destroy_state+0x18/0x30
[  511.876057]  drm_atomic_state_default_clear+0x120/0x2f0
[  511.881316]  drm_atomic_state_clear+0x28/0x30
[  511.885700]  __drm_atomic_state_free+0x18/0x60
[  511.890173]  drm_client_modeset_commit_atomic+0x238/0x280
[  511.895606]  drm_client_modeset_commit_locked+0x50/0x1e0
[  511.900952]  drm_client_modeset_commit+0x2c/0x50
[  511.905599]  drm_fb_helper_restore_fbdev_mode_unlocked+0x74/0xe8
[  511.911643]  drm_fb_helper_set_par+0x2c/0x60
[  511.915941]  drm_fb_helper_hotplug_event.part.20+0xc0/0xd0
[  511.921462]  drm_fbdev_client_hotplug+0xc8/0x1d0
[  511.926109]  drm_client_dev_hotplug+0x78/0xc0
[  511.930494]  drm_kms_helper_hotplug_event+0x30/0x40
[  511.935403]  drm_helper_hpd_irq_event+0x12c/0x160
[  511.940136]  dw_hdmi_irq+0x118/0x1e0
[  511.943735]  irq_thread_fn+0x28/0x88
[  511.947333]  irq_thread+0x13c/0x1d8
[  511.950845]  kthread+0x150/0x158
[  511.954094]  ret_from_fork+0x10/0x18
[  511.957690] irq event stamp: 13016
[  511.961114] hardirqs last  enabled at (13015): [<ffff8000100eecf0>] console_unlock+0x418/0x638
[  511.969781] hardirqs last disabled at (13016): [<ffff800010024918>] do_debug_exception+0x1a0/0x234
[  511.978797] softirqs last  enabled at (13012): [<ffff800010001598>] efi_header_end+0x598/0x5ac
[  511.987464] softirqs last disabled at (13005): [<ffff80001007cf9c>] irq_exit+0x13c/0x148
[  511.995604] ---[ end trace 5cdf3cb912c7ed31 ]---
[  512.000549] vsp1 fea28000.vsp: vsp1_du_pipeline_setup_inputs: failed to setup BRU source
[  522.206891] [drm:drm_atomic_helper_wait_for_flip_done] *ERROR* [CRTC:74:crtc-1] flip_done timed out
[  532.446880] [drm:drm_atomic_helper_wait_for_dependencies] *ERROR* [CRTC:74:crtc-1] flip_done timed out
[  542.686878] [drm:drm_atomic_helper_wait_for_dependencies] *ERROR* [CONNECTOR:80:HDMI-A-1] flip_done timed out
[  552.926877] [drm:drm_atomic_helper_wait_for_dependencies] *ERROR* [PLANE:45:plane-5] flip_done timed out
[  552.926932] vsp1 fea28000.vsp: vsp1_du_pipeline_setup_inputs: failed to setup BRU source
[  563.166888] [drm:drm_atomic_helper_wait_for_flip_done] *ERROR* [CRTC:74:crtc-1] flip_done timed out
[  563.234105] PM: resume of devices complete after 103756.305 msecs
[  563.247877] PM: Finishing wakeup.

Jacopo Mondi July 17, 2020, 3:06 p.m. UTC | #21

Hello Eugeniu,

On Mon, Jun 15, 2020 at 04:17:23PM +0200, Eugeniu Rosca wrote:
> Hi Jacopo,
>
> On Fri, Jun 12, 2020 at 05:12:09PM +0200, Jacopo Mondi wrote:
> > On Mon, Jun 08, 2020 at 11:44:32AM +0200, Eugeniu Rosca wrote:
> > > FWIW, I seem to hit pre-existing issues in vanilla rcar-du,
> > > while unplugging HDMI cable during a cyclic suspend-resume:
> > >
> > > HW: H3 ES2.0 Salvator-X
> > > SW: renesas-drivers-2020-06-02-v5.7
> > > .config: renesas_defconfig +CONFIG_PM_DEBUG +CONFIG_PM_ADVANCED_DEBUG
> > > Use-case:
> > >
> > >   --------8<---------
> > > $ cat s2ram.sh
> > > modprobe i2c-dev
> > > echo 9 > /proc/sys/kernel/printk
> > > i2cset -f -y 7 0x30 0x20 0x0F
> >
> > According to
> > https://elinux.org/R-Car/Boards/Salvator-X#Suspend-to-RAM
> > this is not needed anymore
>
> Good to know. Thanks for the useful remark.
>
> > > echo 0 > /sys/module/suspend/parameters/pm_test_delay
> > > echo core  > /sys/power/pm_test
> > > echo deep > /sys/power/mem_sleep
> > > echo 1 > /sys/power/pm_debug_messages
> > > echo 0 > /sys/power/pm_print_times
> > > echo mem > /sys/power/state
> > >
> > > $ while true; do sh s2ram.sh ; done
> > > $ # unplug HDMI cable several times
> >
> > I tried unplugging an plugging the cable while the system was
> > suspended and after resume but I was not able to reproduce anything.
>
> Your comment sounds like you suspended the system once and resumed it
> afterwards, while my description mentions "cyclic" :), meaning:
>
> $ while true; do sh s2ram.sh; done
> $ # connect-disconnect the hdmi display a couple of times
> $ # NOTE: to avoid this manual step, I am thinking of a USB-controlled
>     HDMI switcher long-term
>
> >
> > Could you provide more precise instructions on how to reproduce this ?
> > I.e. when to disconnect the cable to trigger the below error.
>
> See above :)
>
> BTW, using renesas-drivers-2020-06-02-v5.7 as base and performing the
> use-case just described, I got today (with minimal effort):
>
> [  459.321733] Enabling non-boot CPUs ...
> [  459.331132] Detected PIPT I-cache on CPU1
> [  459.331189] CPU1: Booted secondary processor 0x0000000001 [0x411fd073]
> [  459.332312] CPU1 is up
> [  459.345635] Detected PIPT I-cache on CPU2
> [  459.345671] CPU2: Booted secondary processor 0x0000000002 [0x411fd073]
> [  459.346624] CPU2 is up
> [  459.359912] Detected PIPT I-cache on CPU3
> [  459.359942] CPU3: Booted secondary processor 0x0000000003 [0x411fd073]
> [  459.360918] CPU3 is up
> [  459.374183] Detected VIPT I-cache on CPU4
> [  459.374252] CPU4: Booted secondary processor 0x0000000100 [0x410fd034]
> [  459.375875] cpufreq: cpufreq_online: CPU4: Running at unlisted freq: 1199999 KHz
> [  459.394204] cpufreq: cpufreq_online: CPU4: Unlisted initial frequency changed to: 1200000 KHz
> [  459.403879] CPU4 is up
> [  459.406469] Detected VIPT I-cache on CPU5
> [  459.406519] CPU5: Booted secondary processor 0x0000000101 [0x410fd034]
> [  459.408520] CPU5 is up
> [  459.421762] Detected VIPT I-cache on CPU6
> [  459.421810] CPU6: Booted secondary processor 0x0000000102 [0x410fd034]
> [  459.423831] CPU6 is up
> [  459.437114] Detected VIPT I-cache on CPU7
> [  459.437164] CPU7: Booted secondary processor 0x0000000103 [0x410fd034]
> [  459.439258] CPU7 is up
> [  459.456217] PM: noirq resume of devices complete after 3.878 msecs
> [  459.471529] PM: early resume of devices complete after 8.590 msecs
> [  469.726906] [drm:drm_atomic_helper_wait_for_dependencies] *ERROR* [CRTC:76:crtc-3] flip_done timed out

I've been able to reproduce this same issue, but I see that errors in
drm_atomic_helper_wait_for_dependencies always follow a first failure
in drm_atomic_helper_wait_for_flip_done

Looking at the log what I see is that
[  160.488762] PM: late suspend of devices complete after 10.509 msecs
[  171.235584] [drm:drm_atomic_helper_wait_for_flip_done] *ERROR* [CRTC:75:crtc-1] flip_done timed out

The 10 second elapsed there matches the timout in
drm_atomic_helper_wait_for_flip_done and it seems the issue is related
to the first atomic commit after resume not being able to succesfully
receive a flip_done event, possibly as the HDMI connector has been
disconnected while the system was suspending or suspended and the DRM
state was not updated.

Can you confirm you see the same failure sequence ?

Thanks
  j

Geert Uytterhoeven Aug. 18, 2020, 9:50 a.m. UTC | #22

Hi Laurent,

On Fri, Jun 12, 2020 at 5:51 PM Laurent Pinchart
<[e-mail]> wrote:
> On Fri, Jun 12, 2020 at 05:36:07PM +0200, Eugeniu Rosca wrote:
> > On Fri, Jun 12, 2020 at 06:10:05PM +0300, Laurent Pinchart wrote:
> > > On Fri, Jun 12, 2020 at 05:00:32PM +0200, Jacopo Mondi wrote:
> > > > On Tue, Jun 09, 2020 at 04:29:59PM +0200, Eugeniu Rosca wrote:
> > > > > On Sun, Jun 07, 2020 at 05:41:58AM +0300, Laurent Pinchart wrote:
> > > > > > Note that the CMM driver is controlled by the DU driver. As the DU
> > > > > > driver will reenable the display during resume, it will call
> > > > > > rcar_du_cmm_setup() at resume time, which will reprogram the CMM. There
> > > > > > should thus be no need for manual suspend/resume handling in the CMM as
> > > > > > far as I can tell, but we need to ensure that the CMM is suspended
> > > > > > before and resumed after the DU. I believe this could be implemented
> > > > > > using device links.
> > > > >
> > > > > Based on below quote [*] from Jacopo's commit [**], isn't the device
> > > > > link relationship already in place?
> > > >
> > > > Yes, it's in place already.
> > > >
> > > > I added pm_ops to cmm just to be able to printout when suspend/resume
> > > > happens and the sequence is what comment [*] reports
> > > >
> > > > [  222.909002] rcar_du_pm_suspend:505
> > > > [  223.145497] rcar_cmm_pm_suspend:193
> > > >
> > > > [  223.208053] rcar_cmm_pm_resume:200
> > > > [  223.460094] rcar_du_pm_resume:513
> > > >
> > > > However, Laurent mentioned that in his comment here that he expects
> > > > the opposite sequence to happen (CMM to suspend before and resume after
> > > > DU).
> > > >
> > > > I still think what is implemented is correct:
> > > > - CMM is suspended after DU: when CMM is suspended, DU is not feeding
> > > >   it with data
> > > > - CMM is resumed before: once DU restart operations CMM is ready to
> > > >   receive data.
> > > >
> > > > Laurent, what do you think ?
> > >
> > > I think I shouldn't have written the previous e-mail in the middle of
> > > the night :-) Suspending CMM after DU is obviously correct.
> >
> > Thanks to Renesas team (kudos to Gotthard and Michael), we've
> > figured out that below sequence of clock handling (happening during
> > concurrent suspend and HDMI display unplug) leads to SoC lockup:
> >
> > cmm1 OFF      (caused by HDMI unplug)
> > x21-clock OFF         (caused by HDMI unplug)
> > du1 OFF       (caused by HDMI unplug)
> > cmm1 ON (caused by suspend to ram, as preparation for CMM register save)
> > # Freeze happens
> >
> > That seems to be explained by Chapter 35A.4.3 "Restriction of enabling
> > clock signal of the CMM" of HW User's manual (Rev.2.00 Jul 2019):
> >
> >  -----8<-----
> >  When the clock signal of the CMM is enabled (RMSTPCR7.CMMn or
> >  SMSTPCR7.CMMn = 0), the clock signal of the DU should be also enabled
> >  (RMSTPCR7.DUn or SMSTPCR7.DUn = 0).
> >  -----8<-----
> >
> > So, the lesson learned from the above is: do not enable the CMMi clock
> > while the DUi clock is disabled. I expect this to also potentially
> > give some input w.r.t. what to suspend/resume first, CMM or DU.
>
> This may be an ugly one. The DU driver needs to disable the CMM when
> suspending, and enabling the CMM when resuming. To do so, it calls
> functions of the CMM driver, and those functions access CMM registers.
> We can't do so while the CMM is suspended, so the DU has to suspend
> before the CMM, and resume after the CMM.
>
> On the other hand, as you state here, we need to make sure the CMM clock
> is disabled first. The CMM thus needs to suspend before the DU, and
> resume after the DU.
>
> Those are conflicting requirements.
>
> One option could be to use the .suspend_late() and .resume_early() PM
> operations for the DU, to turn the DU clock off late and turn it back on
> early. Integrating it with the DRM suspend/resume helpers will likely be
> complicated though. I wonder if we could find a more elegant solution.
>
> I the above sequence, you list "cmm1 ON" as a preparation for CMM
> register save. We don't need to save any register, the CMM driver has no
> .suspend() handler. The PM core should really skip waking up the device
> at that point (I recall complaining about the spurious wake ups at
> suspend time a while ago, but that neevr got addressed). Geert, any
> opinion on that ?

If there are issues with the PM core, please bring it up with the PM people.

If there's a (too) close integration of the CMM and the DU, perhaps the
CMM should list the DU module clock in its clocks property, too?
We have a similar construction for USB (module clocks 703 and 704).

Gr{oetje,eeting}s,

                        Geert