[PATCH v3 0/6] drm/vc4: hdmi: Fix CEC access while disabled

[PATCH v3 0/6] drm/vc4: hdmi: Fix CEC access while disabled

[Maxime Ripard]

Hi,

This series aims at fixing a complete and silent hang when one tries to use CEC
while the display output is off.

This can be tested with:

echo off > /sys/class/drm/card0-HDMI-A-1/status
cec-ctl --tuner -p 1.0.0.0
cec-compliance

This series addresses it by making sure the HDMI controller is powered up as
soon as the CEC device is opened by the userspace.

Let me know what you think,
Maxime

Changes from v2:
  - Rebased on top of drm-misc-fixes
  - Fixed a build error

Changes from v1:
  - More fixes
  - Added a big warning if we try to access a register while the device is
    disabled.
  - Fixed the pre_crtc_configure error path

Maxime Ripard (6):
  drm/vc4: select PM
  drm/vc4: hdmi: Make sure the controller is powered up during bind
  drm/vc4: hdmi: Rework the pre_crtc_configure error handling
  drm/vc4: hdmi: Split the CEC disable / enable functions in two
  drm/vc4: hdmi: Make sure the device is powered with CEC
  drm/vc4: hdmi: Warn if we access the controller while disabled

 drivers/gpu/drm/vc4/Kconfig         |   1 +
 drivers/gpu/drm/vc4/vc4_hdmi.c      | 125 ++++++++++++++++++----------
 drivers/gpu/drm/vc4/vc4_hdmi_regs.h |   6 ++
 3 files changed, 90 insertions(+), 42 deletions(-)

-- 
2.31.1

[PATCH v3 1/6] drm/vc4: select PM

[Maxime Ripard]

We already depend on runtime PM to get the power domains and clocks for
most of the devices supported by the vc4 driver, so let's just select it
to make sure it's there, and remove the ifdef.

Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/gpu/drm/vc4/Kconfig    | 1 +
 drivers/gpu/drm/vc4/vc4_hdmi.c | 2 --
 2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/vc4/Kconfig b/drivers/gpu/drm/vc4/Kconfig
index 118e8a426b1a..f774ab340863 100644
--- a/drivers/gpu/drm/vc4/Kconfig
+++ b/drivers/gpu/drm/vc4/Kconfig
@@ -9,6 +9,7 @@ config DRM_VC4
 	select DRM_KMS_CMA_HELPER
 	select DRM_GEM_CMA_HELPER
 	select DRM_PANEL_BRIDGE
+	select PM
 	select SND_PCM
 	select SND_PCM_ELD
 	select SND_SOC_GENERIC_DMAENGINE_PCM
diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
index c2876731ee2d..602203b2d8e1 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.c
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
@@ -2107,7 +2107,6 @@ static int vc5_hdmi_init_resources(struct vc4_hdmi *vc4_hdmi)
 	return 0;
 }
 
-#ifdef CONFIG_PM
 static int vc4_hdmi_runtime_suspend(struct device *dev)
 {
 	struct vc4_hdmi *vc4_hdmi = dev_get_drvdata(dev);
@@ -2128,7 +2127,6 @@ static int vc4_hdmi_runtime_resume(struct device *dev)
 
 	return 0;
 }
-#endif
 
 static int vc4_hdmi_bind(struct device *dev, struct device *master, void *data)
 {
-- 
2.31.1

Re: [PATCH v3 1/6] drm/vc4: select PM

[Dave Stevenson]

On Thu, 19 Aug 2021 at 14:59, Maxime Ripard <maxime@cerno.tech> wrote:
>
> We already depend on runtime PM to get the power domains and clocks for
> most of the devices supported by the vc4 driver, so let's just select it
> to make sure it's there, and remove the ifdef.
>
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>

Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>

> ---
>  drivers/gpu/drm/vc4/Kconfig    | 1 +
>  drivers/gpu/drm/vc4/vc4_hdmi.c | 2 --
>  2 files changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/vc4/Kconfig b/drivers/gpu/drm/vc4/Kconfig
> index 118e8a426b1a..f774ab340863 100644
> --- a/drivers/gpu/drm/vc4/Kconfig
> +++ b/drivers/gpu/drm/vc4/Kconfig
> @@ -9,6 +9,7 @@ config DRM_VC4
>         select DRM_KMS_CMA_HELPER
>         select DRM_GEM_CMA_HELPER
>         select DRM_PANEL_BRIDGE
> +       select PM
>         select SND_PCM
>         select SND_PCM_ELD
>         select SND_SOC_GENERIC_DMAENGINE_PCM
> diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
> index c2876731ee2d..602203b2d8e1 100644
> --- a/drivers/gpu/drm/vc4/vc4_hdmi.c
> +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
> @@ -2107,7 +2107,6 @@ static int vc5_hdmi_init_resources(struct vc4_hdmi *vc4_hdmi)
>         return 0;
>  }
>
> -#ifdef CONFIG_PM
>  static int vc4_hdmi_runtime_suspend(struct device *dev)
>  {
>         struct vc4_hdmi *vc4_hdmi = dev_get_drvdata(dev);
> @@ -2128,7 +2127,6 @@ static int vc4_hdmi_runtime_resume(struct device *dev)
>
>         return 0;
>  }
> -#endif
>
>  static int vc4_hdmi_bind(struct device *dev, struct device *master, void *data)
>  {
> --
> 2.31.1
>

Re: [PATCH v3 1/6] drm/vc4: select PM (openrisc)

[Randy Dunlap]

On 8/19/21 6:59 AM, Maxime Ripard wrote:
> We already depend on runtime PM to get the power domains and clocks for
> most of the devices supported by the vc4 driver, so let's just select it
> to make sure it's there, and remove the ifdef.
> 
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> ---
>   drivers/gpu/drm/vc4/Kconfig    | 1 +
>   drivers/gpu/drm/vc4/vc4_hdmi.c | 2 --
>   2 files changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/vc4/Kconfig b/drivers/gpu/drm/vc4/Kconfig
> index 118e8a426b1a..f774ab340863 100644
> --- a/drivers/gpu/drm/vc4/Kconfig
> +++ b/drivers/gpu/drm/vc4/Kconfig
> @@ -9,6 +9,7 @@ config DRM_VC4
>   	select DRM_KMS_CMA_HELPER
>   	select DRM_GEM_CMA_HELPER
>   	select DRM_PANEL_BRIDGE
> +	select PM
>   	select SND_PCM
>   	select SND_PCM_ELD
>   	select SND_SOC_GENERIC_DMAENGINE_PCM
> diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
> index c2876731ee2d..602203b2d8e1 100644
> --- a/drivers/gpu/drm/vc4/vc4_hdmi.c
> +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
> @@ -2107,7 +2107,6 @@ static int vc5_hdmi_init_resources(struct vc4_hdmi *vc4_hdmi)
>   	return 0;
>   }
>   
> -#ifdef CONFIG_PM
>   static int vc4_hdmi_runtime_suspend(struct device *dev)
>   {
>   	struct vc4_hdmi *vc4_hdmi = dev_get_drvdata(dev);
> @@ -2128,7 +2127,6 @@ static int vc4_hdmi_runtime_resume(struct device *dev)
>   
>   	return 0;
>   }
> -#endif
>   
>   static int vc4_hdmi_bind(struct device *dev, struct device *master, void *data)
>   {
> 

Hi,

FYI.

This still causes a build error on arch/openrisc/ since it does not support
CONFIG_PM (it does not source "kernel/power/Kconfig" like some other arches do):

./arch/riscv/Kconfig:source "kernel/power/Kconfig"
./arch/x86/Kconfig:source "kernel/power/Kconfig"
./arch/nds32/Kconfig:source "kernel/power/Kconfig"
./arch/sh/Kconfig:source "kernel/power/Kconfig"
./arch/arc/Kconfig:source "kernel/power/Kconfig"
./arch/arm64/Kconfig:source "kernel/power/Kconfig"
./arch/xtensa/Kconfig:source "kernel/power/Kconfig"
./arch/sparc/Kconfig:source "kernel/power/Kconfig"
./arch/arm/Kconfig:source "kernel/power/Kconfig"
./arch/mips/Kconfig:source "kernel/power/Kconfig"
./arch/powerpc/Kconfig:source "kernel/power/Kconfig"
./arch/um/Kconfig:source "kernel/power/Kconfig"
./arch/ia64/Kconfig:source "kernel/power/Kconfig"

so with
CONFIG_DRM_VC4=y
# CONFIG_DRM_VC4_HDMI_CEC is not set

I still see
../drivers/gpu/drm/vc4/vc4_hdmi.c:2139:12: warning: 'vc4_hdmi_runtime_suspend' defined but not used [-Wunused-function]
  2139 | static int vc4_hdmi_runtime_suspend(struct device *dev)
       |            ^~~~~~~~~~~~~~~~~~~~~~~~


-- 
~Randy

Re: [PATCH v3 1/6] drm/vc4: select PM (openrisc)

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 3060 bytes --]

Hi Randy,

On Sun, Sep 19, 2021 at 09:40:44AM -0700, Randy Dunlap wrote:
> On 8/19/21 6:59 AM, Maxime Ripard wrote:
> > We already depend on runtime PM to get the power domains and clocks for
> > most of the devices supported by the vc4 driver, so let's just select it
> > to make sure it's there, and remove the ifdef.
> > 
> > Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> > ---
> >   drivers/gpu/drm/vc4/Kconfig    | 1 +
> >   drivers/gpu/drm/vc4/vc4_hdmi.c | 2 --
> >   2 files changed, 1 insertion(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/vc4/Kconfig b/drivers/gpu/drm/vc4/Kconfig
> > index 118e8a426b1a..f774ab340863 100644
> > --- a/drivers/gpu/drm/vc4/Kconfig
> > +++ b/drivers/gpu/drm/vc4/Kconfig
> > @@ -9,6 +9,7 @@ config DRM_VC4
> >   	select DRM_KMS_CMA_HELPER
> >   	select DRM_GEM_CMA_HELPER
> >   	select DRM_PANEL_BRIDGE
> > +	select PM
> >   	select SND_PCM
> >   	select SND_PCM_ELD
> >   	select SND_SOC_GENERIC_DMAENGINE_PCM
> > diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
> > index c2876731ee2d..602203b2d8e1 100644
> > --- a/drivers/gpu/drm/vc4/vc4_hdmi.c
> > +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
> > @@ -2107,7 +2107,6 @@ static int vc5_hdmi_init_resources(struct vc4_hdmi *vc4_hdmi)
> >   	return 0;
> >   }
> > -#ifdef CONFIG_PM
> >   static int vc4_hdmi_runtime_suspend(struct device *dev)
> >   {
> >   	struct vc4_hdmi *vc4_hdmi = dev_get_drvdata(dev);
> > @@ -2128,7 +2127,6 @@ static int vc4_hdmi_runtime_resume(struct device *dev)
> >   	return 0;
> >   }
> > -#endif
> >   static int vc4_hdmi_bind(struct device *dev, struct device *master, void *data)
> >   {
> > 
> 
> Hi,
> 
> FYI.
> 
> This still causes a build error on arch/openrisc/ since it does not support
> CONFIG_PM (it does not source "kernel/power/Kconfig" like some other arches do):
> 
> ./arch/riscv/Kconfig:source "kernel/power/Kconfig"
> ./arch/x86/Kconfig:source "kernel/power/Kconfig"
> ./arch/nds32/Kconfig:source "kernel/power/Kconfig"
> ./arch/sh/Kconfig:source "kernel/power/Kconfig"
> ./arch/arc/Kconfig:source "kernel/power/Kconfig"
> ./arch/arm64/Kconfig:source "kernel/power/Kconfig"
> ./arch/xtensa/Kconfig:source "kernel/power/Kconfig"
> ./arch/sparc/Kconfig:source "kernel/power/Kconfig"
> ./arch/arm/Kconfig:source "kernel/power/Kconfig"
> ./arch/mips/Kconfig:source "kernel/power/Kconfig"
> ./arch/powerpc/Kconfig:source "kernel/power/Kconfig"
> ./arch/um/Kconfig:source "kernel/power/Kconfig"
> ./arch/ia64/Kconfig:source "kernel/power/Kconfig"
> 
> so with
> CONFIG_DRM_VC4=y
> # CONFIG_DRM_VC4_HDMI_CEC is not set
> 
> I still see
> ../drivers/gpu/drm/vc4/vc4_hdmi.c:2139:12: warning: 'vc4_hdmi_runtime_suspend' defined but not used [-Wunused-function]
>  2139 | static int vc4_hdmi_runtime_suspend(struct device *dev)
>       |            ^~~~~~~~~~~~~~~~~~~~~~~~

With what version did you get that build error? -rc2 shouldn't have it
anymore since the runtime_pm hooks introduction got reverted.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v3 1/6] drm/vc4: select PM (openrisc)

[Nathan Chancellor]

On Wed, Sep 22, 2021 at 10:41:56AM +0200, Maxime Ripard wrote:
> Hi Randy,
> 
> On Sun, Sep 19, 2021 at 09:40:44AM -0700, Randy Dunlap wrote:
> > On 8/19/21 6:59 AM, Maxime Ripard wrote:
> > > We already depend on runtime PM to get the power domains and clocks for
> > > most of the devices supported by the vc4 driver, so let's just select it
> > > to make sure it's there, and remove the ifdef.
> > > 
> > > Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> > > ---
> > >   drivers/gpu/drm/vc4/Kconfig    | 1 +
> > >   drivers/gpu/drm/vc4/vc4_hdmi.c | 2 --
> > >   2 files changed, 1 insertion(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/vc4/Kconfig b/drivers/gpu/drm/vc4/Kconfig
> > > index 118e8a426b1a..f774ab340863 100644
> > > --- a/drivers/gpu/drm/vc4/Kconfig
> > > +++ b/drivers/gpu/drm/vc4/Kconfig
> > > @@ -9,6 +9,7 @@ config DRM_VC4
> > >   	select DRM_KMS_CMA_HELPER
> > >   	select DRM_GEM_CMA_HELPER
> > >   	select DRM_PANEL_BRIDGE
> > > +	select PM
> > >   	select SND_PCM
> > >   	select SND_PCM_ELD
> > >   	select SND_SOC_GENERIC_DMAENGINE_PCM
> > > diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
> > > index c2876731ee2d..602203b2d8e1 100644
> > > --- a/drivers/gpu/drm/vc4/vc4_hdmi.c
> > > +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
> > > @@ -2107,7 +2107,6 @@ static int vc5_hdmi_init_resources(struct vc4_hdmi *vc4_hdmi)
> > >   	return 0;
> > >   }
> > > -#ifdef CONFIG_PM
> > >   static int vc4_hdmi_runtime_suspend(struct device *dev)
> > >   {
> > >   	struct vc4_hdmi *vc4_hdmi = dev_get_drvdata(dev);
> > > @@ -2128,7 +2127,6 @@ static int vc4_hdmi_runtime_resume(struct device *dev)
> > >   	return 0;
> > >   }
> > > -#endif
> > >   static int vc4_hdmi_bind(struct device *dev, struct device *master, void *data)
> > >   {
> > > 
> > 
> > Hi,
> > 
> > FYI.
> > 
> > This still causes a build error on arch/openrisc/ since it does not support
> > CONFIG_PM (it does not source "kernel/power/Kconfig" like some other arches do):
> > 
> > ./arch/riscv/Kconfig:source "kernel/power/Kconfig"
> > ./arch/x86/Kconfig:source "kernel/power/Kconfig"
> > ./arch/nds32/Kconfig:source "kernel/power/Kconfig"
> > ./arch/sh/Kconfig:source "kernel/power/Kconfig"
> > ./arch/arc/Kconfig:source "kernel/power/Kconfig"
> > ./arch/arm64/Kconfig:source "kernel/power/Kconfig"
> > ./arch/xtensa/Kconfig:source "kernel/power/Kconfig"
> > ./arch/sparc/Kconfig:source "kernel/power/Kconfig"
> > ./arch/arm/Kconfig:source "kernel/power/Kconfig"
> > ./arch/mips/Kconfig:source "kernel/power/Kconfig"
> > ./arch/powerpc/Kconfig:source "kernel/power/Kconfig"
> > ./arch/um/Kconfig:source "kernel/power/Kconfig"
> > ./arch/ia64/Kconfig:source "kernel/power/Kconfig"
> > 
> > so with
> > CONFIG_DRM_VC4=y
> > # CONFIG_DRM_VC4_HDMI_CEC is not set
> > 
> > I still see
> > ../drivers/gpu/drm/vc4/vc4_hdmi.c:2139:12: warning: 'vc4_hdmi_runtime_suspend' defined but not used [-Wunused-function]
> >  2139 | static int vc4_hdmi_runtime_suspend(struct device *dev)
> >       |            ^~~~~~~~~~~~~~~~~~~~~~~~
> 
> With what version did you get that build error? -rc2 shouldn't have it
> anymore since the runtime_pm hooks introduction got reverted.

-next still contains these patches as Stephen effectively reverted the
changes in Linus' tree when merging in the drm-misc-fixes tree:

https://lore.kernel.org/r/20210920090729.19458953@canb.auug.org.au/

Cheers,
Nathan

Re: [PATCH v3 1/6] drm/vc4: select PM (openrisc)

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 3875 bytes --]

Hi Nathan,

On Wed, Sep 22, 2021 at 08:49:50AM -0700, Nathan Chancellor wrote:
> On Wed, Sep 22, 2021 at 10:41:56AM +0200, Maxime Ripard wrote:
> > Hi Randy,
> > 
> > On Sun, Sep 19, 2021 at 09:40:44AM -0700, Randy Dunlap wrote:
> > > On 8/19/21 6:59 AM, Maxime Ripard wrote:
> > > > We already depend on runtime PM to get the power domains and clocks for
> > > > most of the devices supported by the vc4 driver, so let's just select it
> > > > to make sure it's there, and remove the ifdef.
> > > > 
> > > > Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> > > > ---
> > > >   drivers/gpu/drm/vc4/Kconfig    | 1 +
> > > >   drivers/gpu/drm/vc4/vc4_hdmi.c | 2 --
> > > >   2 files changed, 1 insertion(+), 2 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/vc4/Kconfig b/drivers/gpu/drm/vc4/Kconfig
> > > > index 118e8a426b1a..f774ab340863 100644
> > > > --- a/drivers/gpu/drm/vc4/Kconfig
> > > > +++ b/drivers/gpu/drm/vc4/Kconfig
> > > > @@ -9,6 +9,7 @@ config DRM_VC4
> > > >   	select DRM_KMS_CMA_HELPER
> > > >   	select DRM_GEM_CMA_HELPER
> > > >   	select DRM_PANEL_BRIDGE
> > > > +	select PM
> > > >   	select SND_PCM
> > > >   	select SND_PCM_ELD
> > > >   	select SND_SOC_GENERIC_DMAENGINE_PCM
> > > > diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
> > > > index c2876731ee2d..602203b2d8e1 100644
> > > > --- a/drivers/gpu/drm/vc4/vc4_hdmi.c
> > > > +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
> > > > @@ -2107,7 +2107,6 @@ static int vc5_hdmi_init_resources(struct vc4_hdmi *vc4_hdmi)
> > > >   	return 0;
> > > >   }
> > > > -#ifdef CONFIG_PM
> > > >   static int vc4_hdmi_runtime_suspend(struct device *dev)
> > > >   {
> > > >   	struct vc4_hdmi *vc4_hdmi = dev_get_drvdata(dev);
> > > > @@ -2128,7 +2127,6 @@ static int vc4_hdmi_runtime_resume(struct device *dev)
> > > >   	return 0;
> > > >   }
> > > > -#endif
> > > >   static int vc4_hdmi_bind(struct device *dev, struct device *master, void *data)
> > > >   {
> > > > 
> > > 
> > > Hi,
> > > 
> > > FYI.
> > > 
> > > This still causes a build error on arch/openrisc/ since it does not support
> > > CONFIG_PM (it does not source "kernel/power/Kconfig" like some other arches do):
> > > 
> > > ./arch/riscv/Kconfig:source "kernel/power/Kconfig"
> > > ./arch/x86/Kconfig:source "kernel/power/Kconfig"
> > > ./arch/nds32/Kconfig:source "kernel/power/Kconfig"
> > > ./arch/sh/Kconfig:source "kernel/power/Kconfig"
> > > ./arch/arc/Kconfig:source "kernel/power/Kconfig"
> > > ./arch/arm64/Kconfig:source "kernel/power/Kconfig"
> > > ./arch/xtensa/Kconfig:source "kernel/power/Kconfig"
> > > ./arch/sparc/Kconfig:source "kernel/power/Kconfig"
> > > ./arch/arm/Kconfig:source "kernel/power/Kconfig"
> > > ./arch/mips/Kconfig:source "kernel/power/Kconfig"
> > > ./arch/powerpc/Kconfig:source "kernel/power/Kconfig"
> > > ./arch/um/Kconfig:source "kernel/power/Kconfig"
> > > ./arch/ia64/Kconfig:source "kernel/power/Kconfig"
> > > 
> > > so with
> > > CONFIG_DRM_VC4=y
> > > # CONFIG_DRM_VC4_HDMI_CEC is not set
> > > 
> > > I still see
> > > ../drivers/gpu/drm/vc4/vc4_hdmi.c:2139:12: warning: 'vc4_hdmi_runtime_suspend' defined but not used [-Wunused-function]
> > >  2139 | static int vc4_hdmi_runtime_suspend(struct device *dev)
> > >       |            ^~~~~~~~~~~~~~~~~~~~~~~~
> > 
> > With what version did you get that build error? -rc2 shouldn't have it
> > anymore since the runtime_pm hooks introduction got reverted.
> 
> -next still contains these patches as Stephen effectively reverted the
> changes in Linus' tree when merging in the drm-misc-fixes tree:
> 
> https://lore.kernel.org/r/20210920090729.19458953@canb.auug.org.au/

Ah, indeed, thanks.

What's the typical fix for these errors?

I guess adding a depends on ARM || ARM64 || COMPILE_TEST would work?

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v3 1/6] drm/vc4: select PM (openrisc)

[Nathan Chancellor]

On Thu, Sep 23, 2021 at 04:52:08PM +0200, Maxime Ripard wrote:
> Hi Nathan,
> 
> On Wed, Sep 22, 2021 at 08:49:50AM -0700, Nathan Chancellor wrote:
> > On Wed, Sep 22, 2021 at 10:41:56AM +0200, Maxime Ripard wrote:
> > > Hi Randy,
> > > 
> > > On Sun, Sep 19, 2021 at 09:40:44AM -0700, Randy Dunlap wrote:
> > > > On 8/19/21 6:59 AM, Maxime Ripard wrote:
> > > > > We already depend on runtime PM to get the power domains and clocks for
> > > > > most of the devices supported by the vc4 driver, so let's just select it
> > > > > to make sure it's there, and remove the ifdef.
> > > > > 
> > > > > Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> > > > > ---
> > > > >   drivers/gpu/drm/vc4/Kconfig    | 1 +
> > > > >   drivers/gpu/drm/vc4/vc4_hdmi.c | 2 --
> > > > >   2 files changed, 1 insertion(+), 2 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/vc4/Kconfig b/drivers/gpu/drm/vc4/Kconfig
> > > > > index 118e8a426b1a..f774ab340863 100644
> > > > > --- a/drivers/gpu/drm/vc4/Kconfig
> > > > > +++ b/drivers/gpu/drm/vc4/Kconfig
> > > > > @@ -9,6 +9,7 @@ config DRM_VC4
> > > > >   	select DRM_KMS_CMA_HELPER
> > > > >   	select DRM_GEM_CMA_HELPER
> > > > >   	select DRM_PANEL_BRIDGE
> > > > > +	select PM
> > > > >   	select SND_PCM
> > > > >   	select SND_PCM_ELD
> > > > >   	select SND_SOC_GENERIC_DMAENGINE_PCM
> > > > > diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
> > > > > index c2876731ee2d..602203b2d8e1 100644
> > > > > --- a/drivers/gpu/drm/vc4/vc4_hdmi.c
> > > > > +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
> > > > > @@ -2107,7 +2107,6 @@ static int vc5_hdmi_init_resources(struct vc4_hdmi *vc4_hdmi)
> > > > >   	return 0;
> > > > >   }
> > > > > -#ifdef CONFIG_PM
> > > > >   static int vc4_hdmi_runtime_suspend(struct device *dev)
> > > > >   {
> > > > >   	struct vc4_hdmi *vc4_hdmi = dev_get_drvdata(dev);
> > > > > @@ -2128,7 +2127,6 @@ static int vc4_hdmi_runtime_resume(struct device *dev)
> > > > >   	return 0;
> > > > >   }
> > > > > -#endif
> > > > >   static int vc4_hdmi_bind(struct device *dev, struct device *master, void *data)
> > > > >   {
> > > > > 
> > > > 
> > > > Hi,
> > > > 
> > > > FYI.
> > > > 
> > > > This still causes a build error on arch/openrisc/ since it does not support
> > > > CONFIG_PM (it does not source "kernel/power/Kconfig" like some other arches do):
> > > > 
> > > > ./arch/riscv/Kconfig:source "kernel/power/Kconfig"
> > > > ./arch/x86/Kconfig:source "kernel/power/Kconfig"
> > > > ./arch/nds32/Kconfig:source "kernel/power/Kconfig"
> > > > ./arch/sh/Kconfig:source "kernel/power/Kconfig"
> > > > ./arch/arc/Kconfig:source "kernel/power/Kconfig"
> > > > ./arch/arm64/Kconfig:source "kernel/power/Kconfig"
> > > > ./arch/xtensa/Kconfig:source "kernel/power/Kconfig"
> > > > ./arch/sparc/Kconfig:source "kernel/power/Kconfig"
> > > > ./arch/arm/Kconfig:source "kernel/power/Kconfig"
> > > > ./arch/mips/Kconfig:source "kernel/power/Kconfig"
> > > > ./arch/powerpc/Kconfig:source "kernel/power/Kconfig"
> > > > ./arch/um/Kconfig:source "kernel/power/Kconfig"
> > > > ./arch/ia64/Kconfig:source "kernel/power/Kconfig"
> > > > 
> > > > so with
> > > > CONFIG_DRM_VC4=y
> > > > # CONFIG_DRM_VC4_HDMI_CEC is not set
> > > > 
> > > > I still see
> > > > ../drivers/gpu/drm/vc4/vc4_hdmi.c:2139:12: warning: 'vc4_hdmi_runtime_suspend' defined but not used [-Wunused-function]
> > > >  2139 | static int vc4_hdmi_runtime_suspend(struct device *dev)
> > > >       |            ^~~~~~~~~~~~~~~~~~~~~~~~
> > > 
> > > With what version did you get that build error? -rc2 shouldn't have it
> > > anymore since the runtime_pm hooks introduction got reverted.
> > 
> > -next still contains these patches as Stephen effectively reverted the
> > changes in Linus' tree when merging in the drm-misc-fixes tree:
> > 
> > https://lore.kernel.org/r/20210920090729.19458953@canb.auug.org.au/
> 
> Ah, indeed, thanks.
> 
> What's the typical fix for these errors?
> 
> I guess adding a depends on ARM || ARM64 || COMPILE_TEST would work?

I think the typical fix from most people is marking these functions as
__maybe_unused so that they are always defined but the compiler does not
warn. An alternative would be changing the "select PM" to be
"depends on PM" I believe but that is less frequent.

Cheers,
Nathan

Re: [PATCH v3 1/6] drm/vc4: select PM (openrisc)

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 4923 bytes --]

On Thu, Sep 23, 2021 at 07:55:32AM -0700, Nathan Chancellor wrote:
> On Thu, Sep 23, 2021 at 04:52:08PM +0200, Maxime Ripard wrote:
> > Hi Nathan,
> > 
> > On Wed, Sep 22, 2021 at 08:49:50AM -0700, Nathan Chancellor wrote:
> > > On Wed, Sep 22, 2021 at 10:41:56AM +0200, Maxime Ripard wrote:
> > > > Hi Randy,
> > > > 
> > > > On Sun, Sep 19, 2021 at 09:40:44AM -0700, Randy Dunlap wrote:
> > > > > On 8/19/21 6:59 AM, Maxime Ripard wrote:
> > > > > > We already depend on runtime PM to get the power domains and clocks for
> > > > > > most of the devices supported by the vc4 driver, so let's just select it
> > > > > > to make sure it's there, and remove the ifdef.
> > > > > > 
> > > > > > Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> > > > > > ---
> > > > > >   drivers/gpu/drm/vc4/Kconfig    | 1 +
> > > > > >   drivers/gpu/drm/vc4/vc4_hdmi.c | 2 --
> > > > > >   2 files changed, 1 insertion(+), 2 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/gpu/drm/vc4/Kconfig b/drivers/gpu/drm/vc4/Kconfig
> > > > > > index 118e8a426b1a..f774ab340863 100644
> > > > > > --- a/drivers/gpu/drm/vc4/Kconfig
> > > > > > +++ b/drivers/gpu/drm/vc4/Kconfig
> > > > > > @@ -9,6 +9,7 @@ config DRM_VC4
> > > > > >   	select DRM_KMS_CMA_HELPER
> > > > > >   	select DRM_GEM_CMA_HELPER
> > > > > >   	select DRM_PANEL_BRIDGE
> > > > > > +	select PM
> > > > > >   	select SND_PCM
> > > > > >   	select SND_PCM_ELD
> > > > > >   	select SND_SOC_GENERIC_DMAENGINE_PCM
> > > > > > diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
> > > > > > index c2876731ee2d..602203b2d8e1 100644
> > > > > > --- a/drivers/gpu/drm/vc4/vc4_hdmi.c
> > > > > > +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
> > > > > > @@ -2107,7 +2107,6 @@ static int vc5_hdmi_init_resources(struct vc4_hdmi *vc4_hdmi)
> > > > > >   	return 0;
> > > > > >   }
> > > > > > -#ifdef CONFIG_PM
> > > > > >   static int vc4_hdmi_runtime_suspend(struct device *dev)
> > > > > >   {
> > > > > >   	struct vc4_hdmi *vc4_hdmi = dev_get_drvdata(dev);
> > > > > > @@ -2128,7 +2127,6 @@ static int vc4_hdmi_runtime_resume(struct device *dev)
> > > > > >   	return 0;
> > > > > >   }
> > > > > > -#endif
> > > > > >   static int vc4_hdmi_bind(struct device *dev, struct device *master, void *data)
> > > > > >   {
> > > > > > 
> > > > > 
> > > > > Hi,
> > > > > 
> > > > > FYI.
> > > > > 
> > > > > This still causes a build error on arch/openrisc/ since it does not support
> > > > > CONFIG_PM (it does not source "kernel/power/Kconfig" like some other arches do):
> > > > > 
> > > > > ./arch/riscv/Kconfig:source "kernel/power/Kconfig"
> > > > > ./arch/x86/Kconfig:source "kernel/power/Kconfig"
> > > > > ./arch/nds32/Kconfig:source "kernel/power/Kconfig"
> > > > > ./arch/sh/Kconfig:source "kernel/power/Kconfig"
> > > > > ./arch/arc/Kconfig:source "kernel/power/Kconfig"
> > > > > ./arch/arm64/Kconfig:source "kernel/power/Kconfig"
> > > > > ./arch/xtensa/Kconfig:source "kernel/power/Kconfig"
> > > > > ./arch/sparc/Kconfig:source "kernel/power/Kconfig"
> > > > > ./arch/arm/Kconfig:source "kernel/power/Kconfig"
> > > > > ./arch/mips/Kconfig:source "kernel/power/Kconfig"
> > > > > ./arch/powerpc/Kconfig:source "kernel/power/Kconfig"
> > > > > ./arch/um/Kconfig:source "kernel/power/Kconfig"
> > > > > ./arch/ia64/Kconfig:source "kernel/power/Kconfig"
> > > > > 
> > > > > so with
> > > > > CONFIG_DRM_VC4=y
> > > > > # CONFIG_DRM_VC4_HDMI_CEC is not set
> > > > > 
> > > > > I still see
> > > > > ../drivers/gpu/drm/vc4/vc4_hdmi.c:2139:12: warning: 'vc4_hdmi_runtime_suspend' defined but not used [-Wunused-function]
> > > > >  2139 | static int vc4_hdmi_runtime_suspend(struct device *dev)
> > > > >       |            ^~~~~~~~~~~~~~~~~~~~~~~~
> > > > 
> > > > With what version did you get that build error? -rc2 shouldn't have it
> > > > anymore since the runtime_pm hooks introduction got reverted.
> > > 
> > > -next still contains these patches as Stephen effectively reverted the
> > > changes in Linus' tree when merging in the drm-misc-fixes tree:
> > > 
> > > https://lore.kernel.org/r/20210920090729.19458953@canb.auug.org.au/
> > 
> > Ah, indeed, thanks.
> > 
> > What's the typical fix for these errors?
> > 
> > I guess adding a depends on ARM || ARM64 || COMPILE_TEST would work?
> 
> I think the typical fix from most people is marking these functions as
> __maybe_unused so that they are always defined but the compiler does not
> warn. An alternative would be changing the "select PM" to be
> "depends on PM" I believe but that is less frequent.

Thanks for the suggestion. Since those functions are always going to be
used anyway (but on COMPILE_TEST), I've chosen the opposite approach of
dropping SET_RUNTIME_PM_OPS instead. You're in CC of that patch so feel
free to comment there if you think this is wrong.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[PATCH v3 2/6] drm/vc4: hdmi: Make sure the controller is powered up during bind

[Maxime Ripard]

In the bind hook, we actually need the device to have the HSM clock
running during the final part of the display initialisation where we
reset the controller and initialise the CEC component.

Failing to do so will result in a complete, silent, hang of the CPU.

Fixes: 411efa18e4b0 ("drm/vc4: hdmi: Move the HSM clock enable to runtime_pm")
Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/gpu/drm/vc4/vc4_hdmi.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
index 602203b2d8e1..5dde3e5c1d7f 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.c
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
@@ -2191,6 +2191,18 @@ static int vc4_hdmi_bind(struct device *dev, struct device *master, void *data)
 			vc4_hdmi->disable_4kp60 = true;
 	}
 
+	/*
+	 * We need to have the device powered up at this point to call
+	 * our reset hook and for the CEC init.
+	 */
+	ret = vc4_hdmi_runtime_resume(dev);
+	if (ret)
+		goto err_put_ddc;
+
+	pm_runtime_get_noresume(dev);
+	pm_runtime_set_active(dev);
+	pm_runtime_enable(dev);
+
 	if (vc4_hdmi->variant->reset)
 		vc4_hdmi->variant->reset(vc4_hdmi);
 
@@ -2202,8 +2214,6 @@ static int vc4_hdmi_bind(struct device *dev, struct device *master, void *data)
 		clk_prepare_enable(vc4_hdmi->pixel_bvb_clock);
 	}
 
-	pm_runtime_enable(dev);
-
 	drm_simple_encoder_init(drm, encoder, DRM_MODE_ENCODER_TMDS);
 	drm_encoder_helper_add(encoder, &vc4_hdmi_encoder_helper_funcs);
 
@@ -2223,6 +2233,8 @@ static int vc4_hdmi_bind(struct device *dev, struct device *master, void *data)
 			     vc4_hdmi_debugfs_regs,
 			     vc4_hdmi);
 
+	pm_runtime_put_sync(dev);
+
 	return 0;
 
 err_free_cec:
@@ -2231,6 +2243,7 @@ static int vc4_hdmi_bind(struct device *dev, struct device *master, void *data)
 	vc4_hdmi_connector_destroy(&vc4_hdmi->connector);
 err_destroy_encoder:
 	drm_encoder_cleanup(encoder);
+	pm_runtime_put_sync(dev);
 	pm_runtime_disable(dev);
 err_put_ddc:
 	put_device(&vc4_hdmi->ddc->dev);
-- 
2.31.1

[PATCH v3 3/6] drm/vc4: hdmi: Rework the pre_crtc_configure error handling

[Maxime Ripard]

Since our pre_crtc_configure hook returned void, we didn't implement a
goto-based error path handling, leading to errors like failing to put
back the device in pm_runtime in all the error paths, but also failing
to disable the pixel clock if clk_set_min_rate on the HSM clock fails.

Move to a goto-based implementation to have an easier consitency.

Fixes: 4f6e3d66ac52 ("drm/vc4: Add runtime PM support to the HDMI encoder driver")
Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/gpu/drm/vc4/vc4_hdmi.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
index 5dde3e5c1d7f..8458f38e2883 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.c
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
@@ -913,13 +913,13 @@ static void vc4_hdmi_encoder_pre_crtc_configure(struct drm_encoder *encoder,
 	ret = clk_set_rate(vc4_hdmi->pixel_clock, pixel_rate);
 	if (ret) {
 		DRM_ERROR("Failed to set pixel clock rate: %d\n", ret);
-		return;
+		goto err_put_runtime_pm;
 	}
 
 	ret = clk_prepare_enable(vc4_hdmi->pixel_clock);
 	if (ret) {
 		DRM_ERROR("Failed to turn on pixel clock: %d\n", ret);
-		return;
+		goto err_put_runtime_pm;
 	}
 
 	/*
@@ -942,7 +942,7 @@ static void vc4_hdmi_encoder_pre_crtc_configure(struct drm_encoder *encoder,
 	ret = clk_set_min_rate(vc4_hdmi->hsm_clock, hsm_rate);
 	if (ret) {
 		DRM_ERROR("Failed to set HSM clock rate: %d\n", ret);
-		return;
+		goto err_disable_pixel_clock;
 	}
 
 	vc4_hdmi_cec_update_clk_div(vc4_hdmi);
@@ -957,15 +957,13 @@ static void vc4_hdmi_encoder_pre_crtc_configure(struct drm_encoder *encoder,
 	ret = clk_set_min_rate(vc4_hdmi->pixel_bvb_clock, bvb_rate);
 	if (ret) {
 		DRM_ERROR("Failed to set pixel bvb clock rate: %d\n", ret);
-		clk_disable_unprepare(vc4_hdmi->pixel_clock);
-		return;
+		goto err_disable_pixel_clock;
 	}
 
 	ret = clk_prepare_enable(vc4_hdmi->pixel_bvb_clock);
 	if (ret) {
 		DRM_ERROR("Failed to turn on pixel bvb clock: %d\n", ret);
-		clk_disable_unprepare(vc4_hdmi->pixel_clock);
-		return;
+		goto err_disable_pixel_clock;
 	}
 
 	if (vc4_hdmi->variant->phy_init)
@@ -978,6 +976,15 @@ static void vc4_hdmi_encoder_pre_crtc_configure(struct drm_encoder *encoder,
 
 	if (vc4_hdmi->variant->set_timings)
 		vc4_hdmi->variant->set_timings(vc4_hdmi, conn_state, mode);
+
+	return;
+
+err_disable_pixel_clock:
+	clk_disable_unprepare(vc4_hdmi->pixel_clock);
+err_put_runtime_pm:
+	pm_runtime_put(&vc4_hdmi->pdev->dev);
+
+	return;
 }
 
 static void vc4_hdmi_encoder_pre_crtc_enable(struct drm_encoder *encoder,
-- 
2.31.1

[PATCH v3 4/6] drm/vc4: hdmi: Split the CEC disable / enable functions in two

[Maxime Ripard]

In order to ease further additions to the CEC enable and disable, let's
split the function into two functions, one to enable and the other to
disable.

Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/gpu/drm/vc4/vc4_hdmi.c | 73 ++++++++++++++++++++--------------
 1 file changed, 44 insertions(+), 29 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
index 8458f38e2883..bfa35e32052f 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.c
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
@@ -1740,7 +1740,7 @@ static irqreturn_t vc4_cec_irq_handler(int irq, void *priv)
 	return ret;
 }
 
-static int vc4_hdmi_cec_adap_enable(struct cec_adapter *adap, bool enable)
+static int vc4_hdmi_cec_enable(struct cec_adapter *adap)
 {
 	struct vc4_hdmi *vc4_hdmi = cec_get_drvdata(adap);
 	/* clock period in microseconds */
@@ -1753,38 +1753,53 @@ static int vc4_hdmi_cec_adap_enable(struct cec_adapter *adap, bool enable)
 	val |= ((4700 / usecs) << VC4_HDMI_CEC_CNT_TO_4700_US_SHIFT) |
 	       ((4500 / usecs) << VC4_HDMI_CEC_CNT_TO_4500_US_SHIFT);
 
-	if (enable) {
-		HDMI_WRITE(HDMI_CEC_CNTRL_5, val |
-			   VC4_HDMI_CEC_TX_SW_RESET | VC4_HDMI_CEC_RX_SW_RESET);
-		HDMI_WRITE(HDMI_CEC_CNTRL_5, val);
-		HDMI_WRITE(HDMI_CEC_CNTRL_2,
-			   ((1500 / usecs) << VC4_HDMI_CEC_CNT_TO_1500_US_SHIFT) |
-			   ((1300 / usecs) << VC4_HDMI_CEC_CNT_TO_1300_US_SHIFT) |
-			   ((800 / usecs) << VC4_HDMI_CEC_CNT_TO_800_US_SHIFT) |
-			   ((600 / usecs) << VC4_HDMI_CEC_CNT_TO_600_US_SHIFT) |
-			   ((400 / usecs) << VC4_HDMI_CEC_CNT_TO_400_US_SHIFT));
-		HDMI_WRITE(HDMI_CEC_CNTRL_3,
-			   ((2750 / usecs) << VC4_HDMI_CEC_CNT_TO_2750_US_SHIFT) |
-			   ((2400 / usecs) << VC4_HDMI_CEC_CNT_TO_2400_US_SHIFT) |
-			   ((2050 / usecs) << VC4_HDMI_CEC_CNT_TO_2050_US_SHIFT) |
-			   ((1700 / usecs) << VC4_HDMI_CEC_CNT_TO_1700_US_SHIFT));
-		HDMI_WRITE(HDMI_CEC_CNTRL_4,
-			   ((4300 / usecs) << VC4_HDMI_CEC_CNT_TO_4300_US_SHIFT) |
-			   ((3900 / usecs) << VC4_HDMI_CEC_CNT_TO_3900_US_SHIFT) |
-			   ((3600 / usecs) << VC4_HDMI_CEC_CNT_TO_3600_US_SHIFT) |
-			   ((3500 / usecs) << VC4_HDMI_CEC_CNT_TO_3500_US_SHIFT));
+	HDMI_WRITE(HDMI_CEC_CNTRL_5, val |
+		   VC4_HDMI_CEC_TX_SW_RESET | VC4_HDMI_CEC_RX_SW_RESET);
+	HDMI_WRITE(HDMI_CEC_CNTRL_5, val);
+	HDMI_WRITE(HDMI_CEC_CNTRL_2,
+		   ((1500 / usecs) << VC4_HDMI_CEC_CNT_TO_1500_US_SHIFT) |
+		   ((1300 / usecs) << VC4_HDMI_CEC_CNT_TO_1300_US_SHIFT) |
+		   ((800 / usecs) << VC4_HDMI_CEC_CNT_TO_800_US_SHIFT) |
+		   ((600 / usecs) << VC4_HDMI_CEC_CNT_TO_600_US_SHIFT) |
+		   ((400 / usecs) << VC4_HDMI_CEC_CNT_TO_400_US_SHIFT));
+	HDMI_WRITE(HDMI_CEC_CNTRL_3,
+		   ((2750 / usecs) << VC4_HDMI_CEC_CNT_TO_2750_US_SHIFT) |
+		   ((2400 / usecs) << VC4_HDMI_CEC_CNT_TO_2400_US_SHIFT) |
+		   ((2050 / usecs) << VC4_HDMI_CEC_CNT_TO_2050_US_SHIFT) |
+		   ((1700 / usecs) << VC4_HDMI_CEC_CNT_TO_1700_US_SHIFT));
+	HDMI_WRITE(HDMI_CEC_CNTRL_4,
+		   ((4300 / usecs) << VC4_HDMI_CEC_CNT_TO_4300_US_SHIFT) |
+		   ((3900 / usecs) << VC4_HDMI_CEC_CNT_TO_3900_US_SHIFT) |
+		   ((3600 / usecs) << VC4_HDMI_CEC_CNT_TO_3600_US_SHIFT) |
+		   ((3500 / usecs) << VC4_HDMI_CEC_CNT_TO_3500_US_SHIFT));
+
+	if (!vc4_hdmi->variant->external_irq_controller)
+		HDMI_WRITE(HDMI_CEC_CPU_MASK_CLEAR, VC4_HDMI_CPU_CEC);
 
-		if (!vc4_hdmi->variant->external_irq_controller)
-			HDMI_WRITE(HDMI_CEC_CPU_MASK_CLEAR, VC4_HDMI_CPU_CEC);
-	} else {
-		if (!vc4_hdmi->variant->external_irq_controller)
-			HDMI_WRITE(HDMI_CEC_CPU_MASK_SET, VC4_HDMI_CPU_CEC);
-		HDMI_WRITE(HDMI_CEC_CNTRL_5, val |
-			   VC4_HDMI_CEC_TX_SW_RESET | VC4_HDMI_CEC_RX_SW_RESET);
-	}
 	return 0;
 }
 
+static int vc4_hdmi_cec_disable(struct cec_adapter *adap)
+{
+	struct vc4_hdmi *vc4_hdmi = cec_get_drvdata(adap);
+
+	if (!vc4_hdmi->variant->external_irq_controller)
+		HDMI_WRITE(HDMI_CEC_CPU_MASK_SET, VC4_HDMI_CPU_CEC);
+
+	HDMI_WRITE(HDMI_CEC_CNTRL_5, HDMI_READ(HDMI_CEC_CNTRL_5) |
+		   VC4_HDMI_CEC_TX_SW_RESET | VC4_HDMI_CEC_RX_SW_RESET);
+
+	return 0;
+}
+
+static int vc4_hdmi_cec_adap_enable(struct cec_adapter *adap, bool enable)
+{
+	if (enable)
+		return vc4_hdmi_cec_enable(adap);
+	else
+		return vc4_hdmi_cec_disable(adap);
+}
+
 static int vc4_hdmi_cec_adap_log_addr(struct cec_adapter *adap, u8 log_addr)
 {
 	struct vc4_hdmi *vc4_hdmi = cec_get_drvdata(adap);
-- 
2.31.1

[PATCH v3 5/6] drm/vc4: hdmi: Make sure the device is powered with CEC

[Maxime Ripard]

Similarly to what we encountered with the detect hook with DRM, nothing
actually prevents any of the CEC callback from being run while the HDMI
output is disabled.

However, this is an issue since any register access to the controller
when it's powered down will result in a silent hang.

Let's make sure we run the runtime_pm hooks when the CEC adapter is
opened and closed by the userspace to avoid that issue.

Fixes: 15b4511a4af6 ("drm/vc4: add HDMI CEC support")
Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/gpu/drm/vc4/vc4_hdmi.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
index bfa35e32052f..53647ce902ae 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.c
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
@@ -1745,8 +1745,14 @@ static int vc4_hdmi_cec_enable(struct cec_adapter *adap)
 	struct vc4_hdmi *vc4_hdmi = cec_get_drvdata(adap);
 	/* clock period in microseconds */
 	const u32 usecs = 1000000 / CEC_CLOCK_FREQ;
-	u32 val = HDMI_READ(HDMI_CEC_CNTRL_5);
+	u32 val;
+	int ret;
 
+	ret = pm_runtime_resume_and_get(&vc4_hdmi->pdev->dev);
+	if (ret)
+		return ret;
+
+	val = HDMI_READ(HDMI_CEC_CNTRL_5);
 	val &= ~(VC4_HDMI_CEC_TX_SW_RESET | VC4_HDMI_CEC_RX_SW_RESET |
 		 VC4_HDMI_CEC_CNT_TO_4700_US_MASK |
 		 VC4_HDMI_CEC_CNT_TO_4500_US_MASK);
@@ -1789,6 +1795,8 @@ static int vc4_hdmi_cec_disable(struct cec_adapter *adap)
 	HDMI_WRITE(HDMI_CEC_CNTRL_5, HDMI_READ(HDMI_CEC_CNTRL_5) |
 		   VC4_HDMI_CEC_TX_SW_RESET | VC4_HDMI_CEC_RX_SW_RESET);
 
+	pm_runtime_put(&vc4_hdmi->pdev->dev);
+
 	return 0;
 }
 
-- 
2.31.1

[PATCH v3 6/6] drm/vc4: hdmi: Warn if we access the controller while disabled

[Maxime Ripard]

We've had many silent hangs where the kernel would look like it just
stalled due to the access to one of the HDMI registers while the
controller was disabled.

Add a warning if we're about to do that so that it's at least not silent
anymore.

Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/gpu/drm/vc4/vc4_hdmi_regs.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/vc4/vc4_hdmi_regs.h b/drivers/gpu/drm/vc4/vc4_hdmi_regs.h
index 19d2fdc446bc..99dde6e06a37 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi_regs.h
+++ b/drivers/gpu/drm/vc4/vc4_hdmi_regs.h
@@ -1,6 +1,8 @@
 #ifndef _VC4_HDMI_REGS_H_
 #define _VC4_HDMI_REGS_H_
 
+#include <linux/pm_runtime.h>
+
 #include "vc4_hdmi.h"
 
 #define VC4_HDMI_PACKET_STRIDE			0x24
@@ -412,6 +414,8 @@ static inline u32 vc4_hdmi_read(struct vc4_hdmi *hdmi,
 	const struct vc4_hdmi_variant *variant = hdmi->variant;
 	void __iomem *base;
 
+	WARN_ON(!pm_runtime_active(&hdmi->pdev->dev));
+
 	if (reg >= variant->num_registers) {
 		dev_warn(&hdmi->pdev->dev,
 			 "Invalid register ID %u\n", reg);
@@ -438,6 +442,8 @@ static inline void vc4_hdmi_write(struct vc4_hdmi *hdmi,
 	const struct vc4_hdmi_variant *variant = hdmi->variant;
 	void __iomem *base;
 
+	WARN_ON(!pm_runtime_active(&hdmi->pdev->dev));
+
 	if (reg >= variant->num_registers) {
 		dev_warn(&hdmi->pdev->dev,
 			 "Invalid register ID %u\n", reg);
-- 
2.31.1

Re: [PATCH v3 0/6] drm/vc4: hdmi: Fix CEC access while disabled

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 495 bytes --]

On Thu, Aug 19, 2021 at 03:59:25PM +0200, Maxime Ripard wrote:
> Hi,
> 
> This series aims at fixing a complete and silent hang when one tries to use CEC
> while the display output is off.
> 
> This can be tested with:
> 
> echo off > /sys/class/drm/card0-HDMI-A-1/status
> cec-ctl --tuner -p 1.0.0.0
> cec-compliance
> 
> This series addresses it by making sure the HDMI controller is powered up as
> soon as the CEC device is opened by the userspace.

Applied, thanks!
Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]