[PATCH] drm/etnaviv: make THERMAL selectable

[Emil Velikov]

On 1 December 2017 at 15:35, Lucas Stach <l.stach at 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 at 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 at canb.auug.org.au>
>> > Fixes: dba536cd9538 ("drm/etnaviv: add THERMAL dependency")
>> > Signed-off-by: Philipp Zabel <p.zabel at 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/