diff mbox

drm/etnaviv: make THERMAL selectable

Message ID 20171201143015.946-1-p.zabel@pengutronix.de (mailing list archive)
State New, archived
Headers show

Commit Message

Philipp Zabel Dec. 1, 2017, 2:30 p.m. UTC
Depending on THERMAL || !THERMAL caused a Kconfig dependency loop on
x86_64:

  drivers/gpu/drm/tve200/Kconfig:1:error: recursive dependency detected!
  For a resolution refer to Documentation/kbuild/kconfig-language.txt
  subsection "Kconfig recursive dependency limitations"
  drivers/gpu/drm/tve200/Kconfig:1:       symbol DRM_TVE200 depends on CMA
  For a resolution refer to Documentation/kbuild/kconfig-language.txt
  subsection "Kconfig recursive dependency limitations"
  mm/Kconfig:489: symbol CMA is selected by DRM_ETNAVIV
  For a resolution refer to Documentation/kbuild/kconfig-language.txt
  subsection "Kconfig recursive dependency limitations"
  drivers/gpu/drm/etnaviv/Kconfig:2:      symbol DRM_ETNAVIV depends on THERMAL
  For a resolution refer to Documentation/kbuild/kconfig-language.txt
  subsection "Kconfig recursive dependency limitations"
  drivers/thermal/Kconfig:5:      symbol THERMAL is selected by ACPI_VIDEO
  For a resolution refer to Documentation/kbuild/kconfig-language.txt
  subsection "Kconfig recursive dependency limitations"
  drivers/acpi/Kconfig:189:       symbol ACPI_VIDEO is selected by BACKLIGHT_CLASS_DEVICE
  For a resolution refer to Documentation/kbuild/kconfig-language.txt
  subsection "Kconfig recursive dependency limitations"
  drivers/video/backlight/Kconfig:158:    symbol BACKLIGHT_CLASS_DEVICE is selected by DRM_PARADE_PS8622
  For a resolution refer to Documentation/kbuild/kconfig-language.txt
  subsection "Kconfig recursive dependency limitations"
  drivers/gpu/drm/bridge/Kconfig:62:      symbol DRM_PARADE_PS8622 depends on DRM_BRIDGE
  For a resolution refer to Documentation/kbuild/kconfig-language.txt
  subsection "Kconfig recursive dependency limitations"
  drivers/gpu/drm/bridge/Kconfig:1:       symbol DRM_BRIDGE is selected by DRM_TVE200

To work around this, add a new option to make DRM_ETNAVIV select THERMAL
instead.

Reported-by: Stephen Rothwell <sfr@canb.auug.org.au>
Fixes: dba536cd9538 ("drm/etnaviv: add THERMAL dependency")
Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
 drivers/gpu/drm/etnaviv/Kconfig       | 11 ++++++++++-
 drivers/gpu/drm/etnaviv/etnaviv_gpu.c | 12 +++++++++---
 2 files changed, 19 insertions(+), 4 deletions(-)

Comments

Emil Velikov Dec. 1, 2017, 2:56 p.m. UTC | #1
Hi Philipp,

On 1 December 2017 at 14:30, Philipp Zabel <p.zabel@pengutronix.de> wrote:
> Depending on THERMAL || !THERMAL caused a Kconfig dependency loop on
> x86_64:
>
>   drivers/gpu/drm/tve200/Kconfig:1:error: recursive dependency detected!
>   For a resolution refer to Documentation/kbuild/kconfig-language.txt
>   subsection "Kconfig recursive dependency limitations"
>   drivers/gpu/drm/tve200/Kconfig:1:       symbol DRM_TVE200 depends on CMA
>   For a resolution refer to Documentation/kbuild/kconfig-language.txt
>   subsection "Kconfig recursive dependency limitations"
>   mm/Kconfig:489: symbol CMA is selected by DRM_ETNAVIV
>   For a resolution refer to Documentation/kbuild/kconfig-language.txt
>   subsection "Kconfig recursive dependency limitations"
>   drivers/gpu/drm/etnaviv/Kconfig:2:      symbol DRM_ETNAVIV depends on THERMAL
>   For a resolution refer to Documentation/kbuild/kconfig-language.txt
>   subsection "Kconfig recursive dependency limitations"
>   drivers/thermal/Kconfig:5:      symbol THERMAL is selected by ACPI_VIDEO
>   For a resolution refer to Documentation/kbuild/kconfig-language.txt
>   subsection "Kconfig recursive dependency limitations"
>   drivers/acpi/Kconfig:189:       symbol ACPI_VIDEO is selected by BACKLIGHT_CLASS_DEVICE
>   For a resolution refer to Documentation/kbuild/kconfig-language.txt
>   subsection "Kconfig recursive dependency limitations"
>   drivers/video/backlight/Kconfig:158:    symbol BACKLIGHT_CLASS_DEVICE is selected by DRM_PARADE_PS8622
>   For a resolution refer to Documentation/kbuild/kconfig-language.txt
>   subsection "Kconfig recursive dependency limitations"
>   drivers/gpu/drm/bridge/Kconfig:62:      symbol DRM_PARADE_PS8622 depends on DRM_BRIDGE
>   For a resolution refer to Documentation/kbuild/kconfig-language.txt
>   subsection "Kconfig recursive dependency limitations"
>   drivers/gpu/drm/bridge/Kconfig:1:       symbol DRM_BRIDGE is selected by DRM_TVE200
>
> To work around this, add a new option to make DRM_ETNAVIV select THERMAL
> instead.
>
> Reported-by: Stephen Rothwell <sfr@canb.auug.org.au>
> Fixes: dba536cd9538 ("drm/etnaviv: add THERMAL dependency")
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> ---
>  drivers/gpu/drm/etnaviv/Kconfig       | 11 ++++++++++-
>  drivers/gpu/drm/etnaviv/etnaviv_gpu.c | 12 +++++++++---
>  2 files changed, 19 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/etnaviv/Kconfig b/drivers/gpu/drm/etnaviv/Kconfig
> index bbf2828ca5768..c446e867b3a24 100644
> --- a/drivers/gpu/drm/etnaviv/Kconfig
> +++ b/drivers/gpu/drm/etnaviv/Kconfig
> @@ -4,9 +4,9 @@ config DRM_ETNAVIV
>         depends on DRM
>         depends on ARCH_MXC || ARCH_DOVE || (ARM && COMPILE_TEST)
>         depends on MMU
> -       depends on THERMAL || !THERMAL # if THERMAL=m, this can't be 'y'
>         select SHMEM
>         select SYNC_FILE
> +       select THERMAL if DRM_ETNAVIV_THERMAL
>         select TMPFS
>         select WANT_DEV_COREDUMP
>         select CMA if HAVE_DMA_CONTIGUOUS
> @@ -14,6 +14,15 @@ config DRM_ETNAVIV
>         help
>           DRM driver for Vivante GPUs.
>
> +config DRM_ETNAVIV_THERMAL
> +       bool "enable ETNAVIV thermal throttling"
> +       depends on DRM_ETNAVIV
> +       default y
> +       select THERMAL
Just a quick braindump, while waiting for coffee to kick-in:

Having a separate toggle sounds cool, but yet select(ing) THERMAL, a
user visible option, seems like an abuse.
One should swap the select with the original depends line.
  depends on THERMAL || !THERMAL # if THERMAL=m, this can't be 'y'

That doesn't help with the original circular dependency, yet it seems
that ACPI is also abusing THERMAL.
If one swaps the "select THERMAL" with depends... while guarding the
respective code behind IS_ENABLED things should work like a charm.

HTH
Emil
Lucas Stach Dec. 1, 2017, 3:35 p.m. UTC | #2
Hi Emil,

Am Freitag, den 01.12.2017, 14:56 +0000 schrieb Emil Velikov:
> Hi Philipp,
> 
> On 1 December 2017 at 14:30, Philipp Zabel <p.zabel@pengutronix.de>
> wrote:
> > Depending on THERMAL || !THERMAL caused a Kconfig dependency loop
> > on
> > x86_64:
> > 
> >   drivers/gpu/drm/tve200/Kconfig:1:error: recursive dependency
> > detected!
> >   For a resolution refer to Documentation/kbuild/kconfig-
> > language.txt
> >   subsection "Kconfig recursive dependency limitations"
> >   drivers/gpu/drm/tve200/Kconfig:1:       symbol DRM_TVE200 depends
> > on CMA
> >   For a resolution refer to Documentation/kbuild/kconfig-
> > language.txt
> >   subsection "Kconfig recursive dependency limitations"
> >   mm/Kconfig:489: symbol CMA is selected by DRM_ETNAVIV
> >   For a resolution refer to Documentation/kbuild/kconfig-
> > language.txt
> >   subsection "Kconfig recursive dependency limitations"
> >   drivers/gpu/drm/etnaviv/Kconfig:2:      symbol DRM_ETNAVIV
> > depends on THERMAL
> >   For a resolution refer to Documentation/kbuild/kconfig-
> > language.txt
> >   subsection "Kconfig recursive dependency limitations"
> >   drivers/thermal/Kconfig:5:      symbol THERMAL is selected by
> > ACPI_VIDEO
> >   For a resolution refer to Documentation/kbuild/kconfig-
> > language.txt
> >   subsection "Kconfig recursive dependency limitations"
> >   drivers/acpi/Kconfig:189:       symbol ACPI_VIDEO is selected by
> > BACKLIGHT_CLASS_DEVICE
> >   For a resolution refer to Documentation/kbuild/kconfig-
> > language.txt
> >   subsection "Kconfig recursive dependency limitations"
> >   drivers/video/backlight/Kconfig:158:    symbol
> > BACKLIGHT_CLASS_DEVICE is selected by DRM_PARADE_PS8622
> >   For a resolution refer to Documentation/kbuild/kconfig-
> > language.txt
> >   subsection "Kconfig recursive dependency limitations"
> >   drivers/gpu/drm/bridge/Kconfig:62:      symbol DRM_PARADE_PS8622
> > depends on DRM_BRIDGE
> >   For a resolution refer to Documentation/kbuild/kconfig-
> > language.txt
> >   subsection "Kconfig recursive dependency limitations"
> >   drivers/gpu/drm/bridge/Kconfig:1:       symbol DRM_BRIDGE is
> > selected by DRM_TVE200
> > 
> > To work around this, add a new option to make DRM_ETNAVIV select
> > THERMAL
> > instead.
> > 
> > Reported-by: Stephen Rothwell <sfr@canb.auug.org.au>
> > Fixes: dba536cd9538 ("drm/etnaviv: add THERMAL dependency")
> > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> > ---
> >  drivers/gpu/drm/etnaviv/Kconfig       | 11 ++++++++++-
> >  drivers/gpu/drm/etnaviv/etnaviv_gpu.c | 12 +++++++++---
> >  2 files changed, 19 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/etnaviv/Kconfig
> > b/drivers/gpu/drm/etnaviv/Kconfig
> > index bbf2828ca5768..c446e867b3a24 100644
> > --- a/drivers/gpu/drm/etnaviv/Kconfig
> > +++ b/drivers/gpu/drm/etnaviv/Kconfig
> > @@ -4,9 +4,9 @@ config DRM_ETNAVIV
> >         depends on DRM
> >         depends on ARCH_MXC || ARCH_DOVE || (ARM && COMPILE_TEST)
> >         depends on MMU
> > -       depends on THERMAL || !THERMAL # if THERMAL=m, this can't
> > be 'y'
> >         select SHMEM
> >         select SYNC_FILE
> > +       select THERMAL if DRM_ETNAVIV_THERMAL
> >         select TMPFS
> >         select WANT_DEV_COREDUMP
> >         select CMA if HAVE_DMA_CONTIGUOUS
> > @@ -14,6 +14,15 @@ config DRM_ETNAVIV
> >         help
> >           DRM driver for Vivante GPUs.
> > 
> > +config DRM_ETNAVIV_THERMAL
> > +       bool "enable ETNAVIV thermal throttling"
> > +       depends on DRM_ETNAVIV
> > +       default y
> > +       select THERMAL
> 
> Just a quick braindump, while waiting for coffee to kick-in:
> 
> Having a separate toggle sounds cool, but yet select(ing) THERMAL, a
> user visible option, seems like an abuse.

It's not like this is the first occasion of such a thing in the kernel
Kconfig...

> One should swap the select with the original depends line.
>   depends on THERMAL || !THERMAL # if THERMAL=m, this can't be 'y'
> 
> That doesn't help with the original circular dependency, yet it seems
> that ACPI is also abusing THERMAL.
> If one swaps the "select THERMAL" with depends... while guarding the
> respective code behind IS_ENABLED things should work like a charm.

The intention of the separate symbol is exactly to avoid the dependency
chain while still being able to de-select THERMAL without having to
disable all of etnaviv, but only the thermal throttling support. And if
the user wants thermal throttling support, we better make sure it's
actually available.

Regards,
Lucas
Emil Velikov Dec. 1, 2017, 3:50 p.m. UTC | #3
On 1 December 2017 at 15:35, Lucas Stach <l.stach@pengutronix.de> wrote:
> Hi Emil,
>
> Am Freitag, den 01.12.2017, 14:56 +0000 schrieb Emil Velikov:
>> Hi Philipp,
>>
>> On 1 December 2017 at 14:30, Philipp Zabel <p.zabel@pengutronix.de>
>> wrote:
>> > Depending on THERMAL || !THERMAL caused a Kconfig dependency loop
>> > on
>> > x86_64:
>> >
>> >   drivers/gpu/drm/tve200/Kconfig:1:error: recursive dependency
>> > detected!
>> >   For a resolution refer to Documentation/kbuild/kconfig-
>> > language.txt
>> >   subsection "Kconfig recursive dependency limitations"
>> >   drivers/gpu/drm/tve200/Kconfig:1:       symbol DRM_TVE200 depends
>> > on CMA
>> >   For a resolution refer to Documentation/kbuild/kconfig-
>> > language.txt
>> >   subsection "Kconfig recursive dependency limitations"
>> >   mm/Kconfig:489: symbol CMA is selected by DRM_ETNAVIV
>> >   For a resolution refer to Documentation/kbuild/kconfig-
>> > language.txt
>> >   subsection "Kconfig recursive dependency limitations"
>> >   drivers/gpu/drm/etnaviv/Kconfig:2:      symbol DRM_ETNAVIV
>> > depends on THERMAL
>> >   For a resolution refer to Documentation/kbuild/kconfig-
>> > language.txt
>> >   subsection "Kconfig recursive dependency limitations"
>> >   drivers/thermal/Kconfig:5:      symbol THERMAL is selected by
>> > ACPI_VIDEO
>> >   For a resolution refer to Documentation/kbuild/kconfig-
>> > language.txt
>> >   subsection "Kconfig recursive dependency limitations"
>> >   drivers/acpi/Kconfig:189:       symbol ACPI_VIDEO is selected by
>> > BACKLIGHT_CLASS_DEVICE
>> >   For a resolution refer to Documentation/kbuild/kconfig-
>> > language.txt
>> >   subsection "Kconfig recursive dependency limitations"
>> >   drivers/video/backlight/Kconfig:158:    symbol
>> > BACKLIGHT_CLASS_DEVICE is selected by DRM_PARADE_PS8622
>> >   For a resolution refer to Documentation/kbuild/kconfig-
>> > language.txt
>> >   subsection "Kconfig recursive dependency limitations"
>> >   drivers/gpu/drm/bridge/Kconfig:62:      symbol DRM_PARADE_PS8622
>> > depends on DRM_BRIDGE
>> >   For a resolution refer to Documentation/kbuild/kconfig-
>> > language.txt
>> >   subsection "Kconfig recursive dependency limitations"
>> >   drivers/gpu/drm/bridge/Kconfig:1:       symbol DRM_BRIDGE is
>> > selected by DRM_TVE200
>> >
>> > To work around this, add a new option to make DRM_ETNAVIV select
>> > THERMAL
>> > instead.
>> >
>> > Reported-by: Stephen Rothwell <sfr@canb.auug.org.au>
>> > Fixes: dba536cd9538 ("drm/etnaviv: add THERMAL dependency")
>> > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
>> > ---
>> >  drivers/gpu/drm/etnaviv/Kconfig       | 11 ++++++++++-
>> >  drivers/gpu/drm/etnaviv/etnaviv_gpu.c | 12 +++++++++---
>> >  2 files changed, 19 insertions(+), 4 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/etnaviv/Kconfig
>> > b/drivers/gpu/drm/etnaviv/Kconfig
>> > index bbf2828ca5768..c446e867b3a24 100644
>> > --- a/drivers/gpu/drm/etnaviv/Kconfig
>> > +++ b/drivers/gpu/drm/etnaviv/Kconfig
>> > @@ -4,9 +4,9 @@ config DRM_ETNAVIV
>> >         depends on DRM
>> >         depends on ARCH_MXC || ARCH_DOVE || (ARM && COMPILE_TEST)
>> >         depends on MMU
>> > -       depends on THERMAL || !THERMAL # if THERMAL=m, this can't
>> > be 'y'
>> >         select SHMEM
>> >         select SYNC_FILE
>> > +       select THERMAL if DRM_ETNAVIV_THERMAL
>> >         select TMPFS
>> >         select WANT_DEV_COREDUMP
>> >         select CMA if HAVE_DMA_CONTIGUOUS
>> > @@ -14,6 +14,15 @@ config DRM_ETNAVIV
>> >         help
>> >           DRM driver for Vivante GPUs.
>> >
>> > +config DRM_ETNAVIV_THERMAL
>> > +       bool "enable ETNAVIV thermal throttling"
>> > +       depends on DRM_ETNAVIV
>> > +       default y
>> > +       select THERMAL
>>
>> Just a quick braindump, while waiting for coffee to kick-in:
>>
>> Having a separate toggle sounds cool, but yet select(ing) THERMAL, a
>> user visible option, seems like an abuse.
>
> It's not like this is the first occasion of such a thing in the kernel
> Kconfig...
>
I'm afraid so. Though that doesn't mean it's a good idea ;-)

>> One should swap the select with the original depends line.
>>   depends on THERMAL || !THERMAL # if THERMAL=m, this can't be 'y'
>>
>> That doesn't help with the original circular dependency, yet it seems
>> that ACPI is also abusing THERMAL.
>> If one swaps the "select THERMAL" with depends... while guarding the
>> respective code behind IS_ENABLED things should work like a charm.
>
> The intention of the separate symbol is exactly to avoid the dependency
> chain while still being able to de-select THERMAL without having to
> disable all of etnaviv, but only the thermal throttling support. And if
> the user wants thermal throttling support, we better make sure it's
> actually available.
>
True - having a separate toggle will allows building etnaviv
regardless of THERMAL.
My suggestion seems to align with what Arnd proposed back in April [1]
and July [2].

Just not sure what happened there .. ACPI maintainers seem happy, yet
it never got merged.

-Emil


[1] https://patchwork.freedesktop.org/patch/151317/
[2] https://patchwork.freedesktop.org/patch/169164/
diff mbox

Patch

diff --git a/drivers/gpu/drm/etnaviv/Kconfig b/drivers/gpu/drm/etnaviv/Kconfig
index bbf2828ca5768..c446e867b3a24 100644
--- a/drivers/gpu/drm/etnaviv/Kconfig
+++ b/drivers/gpu/drm/etnaviv/Kconfig
@@ -4,9 +4,9 @@  config DRM_ETNAVIV
 	depends on DRM
 	depends on ARCH_MXC || ARCH_DOVE || (ARM && COMPILE_TEST)
 	depends on MMU
-	depends on THERMAL || !THERMAL # if THERMAL=m, this can't be 'y'
 	select SHMEM
 	select SYNC_FILE
+	select THERMAL if DRM_ETNAVIV_THERMAL
 	select TMPFS
 	select WANT_DEV_COREDUMP
 	select CMA if HAVE_DMA_CONTIGUOUS
@@ -14,6 +14,15 @@  config DRM_ETNAVIV
 	help
 	  DRM driver for Vivante GPUs.
 
+config DRM_ETNAVIV_THERMAL
+	bool "enable ETNAVIV thermal throttling"
+	depends on DRM_ETNAVIV
+	default y
+	select THERMAL
+	help
+	  Compile in support for thermal throttling.
+	  Say Y unless you want to risk burning your SoC.
+
 config DRM_ETNAVIV_REGISTER_LOGGING
 	bool "enable ETNAVIV register logging"
 	depends on DRM_ETNAVIV
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
index e19cbe05da2a3..4196f03cf1eb6 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
@@ -31,6 +31,10 @@ 
 #include "state_hi.xml.h"
 #include "cmdstream.xml.h"
 
+#if IS_ENABLED(CONFIG_THERMAL) && !IS_ENABLED(CONFIG_DRM_ETNAVIV_THERMAL)
+#warning "You may want to enable CONFIG_DRM_ETNAVIV_THERMAL."
+#endif
+
 static const struct platform_device_id gpu_ids[] = {
 	{ .name = "etnaviv-gpu,2d" },
 	{ },
@@ -1738,7 +1742,7 @@  static int etnaviv_gpu_bind(struct device *dev, struct device *master,
 	struct etnaviv_gpu *gpu = dev_get_drvdata(dev);
 	int ret;
 
-	if (IS_ENABLED(CONFIG_THERMAL)) {
+	if (IS_ENABLED(CONFIG_DRM_ETNAVIV_THERMAL)) {
 		gpu->cooling = thermal_of_cooling_device_register(dev->of_node,
 				(char *)dev_name(dev), gpu, &cooling_ops);
 		if (IS_ERR(gpu->cooling))
@@ -1751,7 +1755,8 @@  static int etnaviv_gpu_bind(struct device *dev, struct device *master,
 	ret = etnaviv_gpu_clk_enable(gpu);
 #endif
 	if (ret < 0) {
-		thermal_cooling_device_unregister(gpu->cooling);
+		if (IS_ENABLED(CONFIG_DRM_ETNAVIV_THERMAL))
+			thermal_cooling_device_unregister(gpu->cooling);
 		return ret;
 	}
 
@@ -1808,7 +1813,8 @@  static void etnaviv_gpu_unbind(struct device *dev, struct device *master,
 
 	gpu->drm = NULL;
 
-	thermal_cooling_device_unregister(gpu->cooling);
+	if (IS_ENABLED(CONFIG_DRM_ETNAVIV_THERMAL))
+		thermal_cooling_device_unregister(gpu->cooling);
 	gpu->cooling = NULL;
 }