[PATCH] drm/panel: add CONFIG_DRM_KMS_HELPER dependencies

[PATCH] drm/panel: add CONFIG_DRM_KMS_HELPER dependencies

[Arnd Bergmann]

From: Arnd Bergmann <arnd@arndb.de>

The driver fails to build when the KMS helpers are disabled:

ld.lld: error: undefined symbol: drm_gem_fb_get_obj
>>> referenced by drm_mipi_dbi.c
>>>               gpu/drm/drm_mipi_dbi.o:(mipi_dbi_buf_copy) in archive drivers/built-in.a
>>> referenced by drm_mipi_dbi.c
>>>               gpu/drm/drm_mipi_dbi.o:(mipi_dbi_fb_dirty) in archive drivers/built-in.a

ld.lld: error: undefined symbol: drm_gem_fb_begin_cpu_access
>>> referenced by drm_mipi_dbi.c
>>>               gpu/drm/drm_mipi_dbi.o:(mipi_dbi_buf_copy) in archive drivers/built-in.a

ld.lld: error: undefined symbol: drm_fb_swab
>>> referenced by drm_mipi_dbi.c
>>>               gpu/drm/drm_mipi_dbi.o:(mipi_dbi_buf_copy) in archive drivers/built-in.a

ld.lld: error: undefined symbol: drm_fb_xrgb8888_to_rgb565
>>> referenced by drm_mipi_dbi.c
>>>               gpu/drm/drm_mipi_dbi.o:(mipi_dbi_buf_copy) in archive drivers/built-in.a

ld.lld: error: undefined symbol: drm_fb_memcpy
>>> referenced by drm_mipi_dbi.c
>>>               gpu/drm/drm_mipi_dbi.o:(mipi_dbi_buf_copy) in archive drivers/built-in.a

This is fairly hard to hit in randconfig drivers, but it eventually
did trigger for me in a configuration where all other DRM drivers
are loadable modules, but DRM_PANEL_WIDECHIPS_WS2401 was built-in.

Adding a dependency in all drivers that select DRM_MIPI_DBI avoids
the problem for now, adding the dependency in DRM_MIPI_DBI as well
should help make it easier to figure out why it breaks if someone
forgets the dependency the next time.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/gpu/drm/Kconfig       | 2 +-
 drivers/gpu/drm/panel/Kconfig | 4 ++++
 2 files changed, 5 insertions(+), 1 deletion(-)

I see this warning on 5.17-rc8, but did not test it on linux-next,
which may already have a fix.


diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index b1f22e457fd0..d5ec0b77c010 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -30,7 +30,7 @@ menuconfig DRM
 
 config DRM_MIPI_DBI
 	tristate
-	depends on DRM
+	depends on DRM_KMS_HELPER
 
 config DRM_MIPI_DSI
 	bool
diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
index 0aec5a10b064..96887d0efb9f 100644
--- a/drivers/gpu/drm/panel/Kconfig
+++ b/drivers/gpu/drm/panel/Kconfig
@@ -425,6 +425,7 @@ config DRM_PANEL_SAMSUNG_DB7430
 	tristate "Samsung DB7430-based DPI panels"
 	depends on OF && SPI && GPIOLIB
 	depends on BACKLIGHT_CLASS_DEVICE
+	depends on DRM_KMS_HELPER
 	select DRM_MIPI_DBI
 	help
 	  Say Y here if you want to enable support for the Samsung
@@ -440,6 +441,7 @@ config DRM_PANEL_SAMSUNG_S6D16D0
 config DRM_PANEL_SAMSUNG_S6D27A1
 	tristate "Samsung S6D27A1 DPI panel driver"
 	depends on OF && SPI && GPIOLIB
+	depends on DRM_KMS_HELPER
 	select DRM_MIPI_DBI
 	help
 	  Say Y here if you want to enable support for the Samsung
@@ -476,6 +478,7 @@ config DRM_PANEL_SAMSUNG_S6E63M0_SPI
 	depends on SPI
 	depends on DRM_PANEL_SAMSUNG_S6E63M0
 	default DRM_PANEL_SAMSUNG_S6E63M0
+	depends on DRM_KMS_HELPER
 	select DRM_MIPI_DBI
 	help
 	  Say Y here if you want to be able to access the Samsung
@@ -677,6 +680,7 @@ config DRM_PANEL_WIDECHIPS_WS2401
 	tristate "Widechips WS2401 DPI panel driver"
 	depends on SPI && GPIOLIB
 	depends on BACKLIGHT_CLASS_DEVICE
+	depends on DRM_KMS_HELPER
 	select DRM_MIPI_DBI
 	help
 	  Say Y here if you want to enable support for the Widechips WS2401 DPI
-- 
2.29.2

[PATCH] drm/tegra: vic: fix unused-function warnings

[Arnd Bergmann]

From: Arnd Bergmann <arnd@arndb.de>

The use of the old-style SET_RUNTIME_PM_OPS() and
SET_SYSTEM_SLEEP_PM_OPS() macros requires function definitions
to be hidden to avoid

drivers/gpu/drm/tegra/vic.c:326:12: error: 'vic_runtime_suspend' defined but not used [-Werror=unused-function]
  326 | static int vic_runtime_suspend(struct device *dev)
      |            ^~~~~~~~~~~~~~~~~~~
drivers/gpu/drm/tegra/vic.c:292:12: error: 'vic_runtime_resume' defined but not used [-Werror=unused-function]
  292 | static int vic_runtime_resume(struct device *dev)
      |            ^~~~~~~~~~~~~~~~~~

Use the new-style SYSTEM_SLEEP_PM_OPS() and RUNTIME_PM_OPS() instead.

Fixes: 1e15f5b911d6 ("drm/tegra: vic: Stop channel on suspend")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/gpu/drm/tegra/vic.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

I see this warning on 5.17-rc8, but did not test it on linux-next,
which may already have a fix.

diff --git a/drivers/gpu/drm/tegra/vic.c b/drivers/gpu/drm/tegra/vic.c
index 1e342fa3d27b..f56f5921a8c2 100644
--- a/drivers/gpu/drm/tegra/vic.c
+++ b/drivers/gpu/drm/tegra/vic.c
@@ -513,9 +513,8 @@ static int vic_remove(struct platform_device *pdev)
 }
 
 static const struct dev_pm_ops vic_pm_ops = {
-	SET_RUNTIME_PM_OPS(vic_runtime_suspend, vic_runtime_resume, NULL)
-	SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
-				pm_runtime_force_resume)
+	RUNTIME_PM_OPS(vic_runtime_suspend, vic_runtime_resume, NULL)
+	SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, pm_runtime_force_resume)
 };
 
 struct platform_driver tegra_vic_driver = {
-- 
2.29.2

Re: [PATCH] drm/tegra: vic: fix unused-function warnings

[Thierry Reding]

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

On Wed, Mar 16, 2022 at 07:36:47PM +0100, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> 
> The use of the old-style SET_RUNTIME_PM_OPS() and
> SET_SYSTEM_SLEEP_PM_OPS() macros requires function definitions
> to be hidden to avoid
> 
> drivers/gpu/drm/tegra/vic.c:326:12: error: 'vic_runtime_suspend' defined but not used [-Werror=unused-function]
>   326 | static int vic_runtime_suspend(struct device *dev)
>       |            ^~~~~~~~~~~~~~~~~~~
> drivers/gpu/drm/tegra/vic.c:292:12: error: 'vic_runtime_resume' defined but not used [-Werror=unused-function]
>   292 | static int vic_runtime_resume(struct device *dev)
>       |            ^~~~~~~~~~~~~~~~~~
> 
> Use the new-style SYSTEM_SLEEP_PM_OPS() and RUNTIME_PM_OPS() instead.
> 
> Fixes: 1e15f5b911d6 ("drm/tegra: vic: Stop channel on suspend")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  drivers/gpu/drm/tegra/vic.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> I see this warning on 5.17-rc8, but did not test it on linux-next,
> which may already have a fix.
> 
> diff --git a/drivers/gpu/drm/tegra/vic.c b/drivers/gpu/drm/tegra/vic.c
> index 1e342fa3d27b..f56f5921a8c2 100644
> --- a/drivers/gpu/drm/tegra/vic.c
> +++ b/drivers/gpu/drm/tegra/vic.c
> @@ -513,9 +513,8 @@ static int vic_remove(struct platform_device *pdev)
>  }
>  
>  static const struct dev_pm_ops vic_pm_ops = {
> -	SET_RUNTIME_PM_OPS(vic_runtime_suspend, vic_runtime_resume, NULL)
> -	SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
> -				pm_runtime_force_resume)
> +	RUNTIME_PM_OPS(vic_runtime_suspend, vic_runtime_resume, NULL)
> +	SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, pm_runtime_force_resume)
>  };
>  
>  struct platform_driver tegra_vic_driver = {

Hi Arnd,

is this a replacement for __maybe_unused annotations that we would
typically use to address these? Is the ternary operator in PTR_IF enough
to eliminate the warning? Does that work the same way for structure
definitions as it does for conditionals where we use IS_ENABLED() to use
the compiler's DCE for improved coverage?

It looks like it, but just making sure because there's another patch
that fixes this warning by adding __maybe_unused.

Thierry

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

Re: [PATCH] drm/tegra: vic: fix unused-function warnings

[Arnd Bergmann]

On Wed, Apr 6, 2022 at 3:21 PM Thierry Reding <thierry.reding@gmail.com> wrote:
> On Wed, Mar 16, 2022 at 07:36:47PM +0100, Arnd Bergmann wrote:
> >  static const struct dev_pm_ops vic_pm_ops = {
> > -     SET_RUNTIME_PM_OPS(vic_runtime_suspend, vic_runtime_resume, NULL)
> > -     SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
> > -                             pm_runtime_force_resume)
> > +     RUNTIME_PM_OPS(vic_runtime_suspend, vic_runtime_resume, NULL)
> > +     SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, pm_runtime_force_resume)
> >  };
> >
> >  struct platform_driver tegra_vic_driver = {
>
> Hi Arnd,
>
> is this a replacement for __maybe_unused annotations that we would
> typically use to address these? Is the ternary operator in PTR_IF enough
> to eliminate the warning? Does that work the same way for structure
> definitions as it does for conditionals where we use IS_ENABLED() to use
> the compiler's DCE for improved coverage?

Yes to all three.

> It looks like it, but just making sure because there's another patch
> that fixes this warning by adding __maybe_unused.

I sent a lot of patches in the past to add __maybe_unused, but this was
mainly because we could never come up with a good replacement.
Paul Cercueil has finally come up with a good solution, so this is how
we should do it from now on.

        Arnd

Re: [PATCH] drm/panel: add CONFIG_DRM_KMS_HELPER dependencies

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 4335 bytes --]

Hi

Am 16.03.22 um 19:36 schrieb Arnd Bergmann:
> From: Arnd Bergmann <arnd@arndb.de>
> 
> The driver fails to build when the KMS helpers are disabled:
> 
> ld.lld: error: undefined symbol: drm_gem_fb_get_obj
>>>> referenced by drm_mipi_dbi.c
>>>>                gpu/drm/drm_mipi_dbi.o:(mipi_dbi_buf_copy) in archive drivers/built-in.a
>>>> referenced by drm_mipi_dbi.c
>>>>                gpu/drm/drm_mipi_dbi.o:(mipi_dbi_fb_dirty) in archive drivers/built-in.a
> 
> ld.lld: error: undefined symbol: drm_gem_fb_begin_cpu_access
>>>> referenced by drm_mipi_dbi.c
>>>>                gpu/drm/drm_mipi_dbi.o:(mipi_dbi_buf_copy) in archive drivers/built-in.a
> 
> ld.lld: error: undefined symbol: drm_fb_swab
>>>> referenced by drm_mipi_dbi.c
>>>>                gpu/drm/drm_mipi_dbi.o:(mipi_dbi_buf_copy) in archive drivers/built-in.a
> 
> ld.lld: error: undefined symbol: drm_fb_xrgb8888_to_rgb565
>>>> referenced by drm_mipi_dbi.c
>>>>                gpu/drm/drm_mipi_dbi.o:(mipi_dbi_buf_copy) in archive drivers/built-in.a
> 
> ld.lld: error: undefined symbol: drm_fb_memcpy
>>>> referenced by drm_mipi_dbi.c
>>>>                gpu/drm/drm_mipi_dbi.o:(mipi_dbi_buf_copy) in archive drivers/built-in.a
> 
> This is fairly hard to hit in randconfig drivers, but it eventually
> did trigger for me in a configuration where all other DRM drivers
> are loadable modules, but DRM_PANEL_WIDECHIPS_WS2401 was built-in.
> 
> Adding a dependency in all drivers that select DRM_MIPI_DBI avoids
> the problem for now, adding the dependency in DRM_MIPI_DBI as well
> should help make it easier to figure out why it breaks if someone
> forgets the dependency the next time.
> 
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>   drivers/gpu/drm/Kconfig       | 2 +-
>   drivers/gpu/drm/panel/Kconfig | 4 ++++
>   2 files changed, 5 insertions(+), 1 deletion(-)
> 
> I see this warning on 5.17-rc8, but did not test it on linux-next,
> which may already have a fix.
> 
> 
> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> index b1f22e457fd0..d5ec0b77c010 100644
> --- a/drivers/gpu/drm/Kconfig
> +++ b/drivers/gpu/drm/Kconfig
> @@ -30,7 +30,7 @@ menuconfig DRM
>   
>   config DRM_MIPI_DBI
>   	tristate
> -	depends on DRM
> +	depends on DRM_KMS_HELPER

This symbol cannot be selected by users, so it's maybe not a good idea 
to depend on it. In fact, I've had to remove such a statement because it 
created a cyclic dependency. [1]

Making the drivers depend on KMS helpers is the right thing though. If 
there's a better solution, please let me know.

Best regards
Thomas

>   
>   config DRM_MIPI_DSI
>   	bool
> diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
> index 0aec5a10b064..96887d0efb9f 100644
> --- a/drivers/gpu/drm/panel/Kconfig
> +++ b/drivers/gpu/drm/panel/Kconfig
> @@ -425,6 +425,7 @@ config DRM_PANEL_SAMSUNG_DB7430
>   	tristate "Samsung DB7430-based DPI panels"
>   	depends on OF && SPI && GPIOLIB
>   	depends on BACKLIGHT_CLASS_DEVICE
> +	depends on DRM_KMS_HELPER
>   	select DRM_MIPI_DBI
>   	help
>   	  Say Y here if you want to enable support for the Samsung
> @@ -440,6 +441,7 @@ config DRM_PANEL_SAMSUNG_S6D16D0
>   config DRM_PANEL_SAMSUNG_S6D27A1
>   	tristate "Samsung S6D27A1 DPI panel driver"
>   	depends on OF && SPI && GPIOLIB
> +	depends on DRM_KMS_HELPER
>   	select DRM_MIPI_DBI
>   	help
>   	  Say Y here if you want to enable support for the Samsung
> @@ -476,6 +478,7 @@ config DRM_PANEL_SAMSUNG_S6E63M0_SPI
>   	depends on SPI
>   	depends on DRM_PANEL_SAMSUNG_S6E63M0
>   	default DRM_PANEL_SAMSUNG_S6E63M0
> +	depends on DRM_KMS_HELPER
>   	select DRM_MIPI_DBI
>   	help
>   	  Say Y here if you want to be able to access the Samsung
> @@ -677,6 +680,7 @@ config DRM_PANEL_WIDECHIPS_WS2401
>   	tristate "Widechips WS2401 DPI panel driver"
>   	depends on SPI && GPIOLIB
>   	depends on BACKLIGHT_CLASS_DEVICE
> +	depends on DRM_KMS_HELPER
>   	select DRM_MIPI_DBI
>   	help
>   	  Say Y here if you want to enable support for the Widechips WS2401 DPI

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH] drm/panel: add CONFIG_DRM_KMS_HELPER dependencies

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 4975 bytes --]



Am 16.03.22 um 20:12 schrieb Thomas Zimmermann:
> Hi
> 
> Am 16.03.22 um 19:36 schrieb Arnd Bergmann:
>> From: Arnd Bergmann <arnd@arndb.de>
>>
>> The driver fails to build when the KMS helpers are disabled:
>>
>> ld.lld: error: undefined symbol: drm_gem_fb_get_obj
>>>>> referenced by drm_mipi_dbi.c
>>>>>                gpu/drm/drm_mipi_dbi.o:(mipi_dbi_buf_copy) in 
>>>>> archive drivers/built-in.a
>>>>> referenced by drm_mipi_dbi.c
>>>>>                gpu/drm/drm_mipi_dbi.o:(mipi_dbi_fb_dirty) in 
>>>>> archive drivers/built-in.a
>>
>> ld.lld: error: undefined symbol: drm_gem_fb_begin_cpu_access
>>>>> referenced by drm_mipi_dbi.c
>>>>>                gpu/drm/drm_mipi_dbi.o:(mipi_dbi_buf_copy) in 
>>>>> archive drivers/built-in.a
>>
>> ld.lld: error: undefined symbol: drm_fb_swab
>>>>> referenced by drm_mipi_dbi.c
>>>>>                gpu/drm/drm_mipi_dbi.o:(mipi_dbi_buf_copy) in 
>>>>> archive drivers/built-in.a
>>
>> ld.lld: error: undefined symbol: drm_fb_xrgb8888_to_rgb565
>>>>> referenced by drm_mipi_dbi.c
>>>>>                gpu/drm/drm_mipi_dbi.o:(mipi_dbi_buf_copy) in 
>>>>> archive drivers/built-in.a
>>
>> ld.lld: error: undefined symbol: drm_fb_memcpy
>>>>> referenced by drm_mipi_dbi.c
>>>>>                gpu/drm/drm_mipi_dbi.o:(mipi_dbi_buf_copy) in 
>>>>> archive drivers/built-in.a
>>
>> This is fairly hard to hit in randconfig drivers, but it eventually
>> did trigger for me in a configuration where all other DRM drivers
>> are loadable modules, but DRM_PANEL_WIDECHIPS_WS2401 was built-in.
>>
>> Adding a dependency in all drivers that select DRM_MIPI_DBI avoids
>> the problem for now, adding the dependency in DRM_MIPI_DBI as well
>> should help make it easier to figure out why it breaks if someone
>> forgets the dependency the next time.
>>
>> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>> ---
>>   drivers/gpu/drm/Kconfig       | 2 +-
>>   drivers/gpu/drm/panel/Kconfig | 4 ++++
>>   2 files changed, 5 insertions(+), 1 deletion(-)
>>
>> I see this warning on 5.17-rc8, but did not test it on linux-next,
>> which may already have a fix.
>>
>>
>> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
>> index b1f22e457fd0..d5ec0b77c010 100644
>> --- a/drivers/gpu/drm/Kconfig
>> +++ b/drivers/gpu/drm/Kconfig
>> @@ -30,7 +30,7 @@ menuconfig DRM
>>   config DRM_MIPI_DBI
>>       tristate
>> -    depends on DRM
>> +    depends on DRM_KMS_HELPER
> 
> This symbol cannot be selected by users, so it's maybe not a good idea 
> to depend on it. In fact, I've had to remove such a statement because it 
> created a cyclic dependency. [1]

[1] 
https://lore.kernel.org/dri-devel/20220315084559.23510-1-tzimmermann@suse.de/

> 
> Making the drivers depend on KMS helpers is the right thing though. If 
> there's a better solution, please let me know.
> 
> Best regards
> Thomas
> 
>>   config DRM_MIPI_DSI
>>       bool
>> diff --git a/drivers/gpu/drm/panel/Kconfig 
>> b/drivers/gpu/drm/panel/Kconfig
>> index 0aec5a10b064..96887d0efb9f 100644
>> --- a/drivers/gpu/drm/panel/Kconfig
>> +++ b/drivers/gpu/drm/panel/Kconfig
>> @@ -425,6 +425,7 @@ config DRM_PANEL_SAMSUNG_DB7430
>>       tristate "Samsung DB7430-based DPI panels"
>>       depends on OF && SPI && GPIOLIB
>>       depends on BACKLIGHT_CLASS_DEVICE
>> +    depends on DRM_KMS_HELPER
>>       select DRM_MIPI_DBI
>>       help
>>         Say Y here if you want to enable support for the Samsung
>> @@ -440,6 +441,7 @@ config DRM_PANEL_SAMSUNG_S6D16D0
>>   config DRM_PANEL_SAMSUNG_S6D27A1
>>       tristate "Samsung S6D27A1 DPI panel driver"
>>       depends on OF && SPI && GPIOLIB
>> +    depends on DRM_KMS_HELPER
>>       select DRM_MIPI_DBI
>>       help
>>         Say Y here if you want to enable support for the Samsung
>> @@ -476,6 +478,7 @@ config DRM_PANEL_SAMSUNG_S6E63M0_SPI
>>       depends on SPI
>>       depends on DRM_PANEL_SAMSUNG_S6E63M0
>>       default DRM_PANEL_SAMSUNG_S6E63M0
>> +    depends on DRM_KMS_HELPER
>>       select DRM_MIPI_DBI
>>       help
>>         Say Y here if you want to be able to access the Samsung
>> @@ -677,6 +680,7 @@ config DRM_PANEL_WIDECHIPS_WS2401
>>       tristate "Widechips WS2401 DPI panel driver"
>>       depends on SPI && GPIOLIB
>>       depends on BACKLIGHT_CLASS_DEVICE
>> +    depends on DRM_KMS_HELPER
>>       select DRM_MIPI_DBI
>>       help
>>         Say Y here if you want to enable support for the Widechips 
>> WS2401 DPI
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH] drm/panel: add CONFIG_DRM_KMS_HELPER dependencies

[Arnd Bergmann]

On Wed, Mar 16, 2022 at 8:31 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
> Am 16.03.22 um 20:12 schrieb Thomas Zimmermann:
> >>
> >> Adding a dependency in all drivers that select DRM_MIPI_DBI avoids
> >> the problem for now, adding the dependency in DRM_MIPI_DBI as well
> >> should help make it easier to figure out why it breaks if someone
> >> forgets the dependency the next time.
> >>
> >>       tristate
> >> -    depends on DRM
> >> +    depends on DRM_KMS_HELPER
> >
> > This symbol cannot be selected by users, so it's maybe not a good idea
> > to depend on it. In fact, I've had to remove such a statement because it
> > created a cyclic dependency. [1]

I tried to explain above what I was thinking here: the added dependency
is both a correct statement (DRM_MIPI_DBI depends on DRM_KMS_HELPER
because it cannot be built without DRM_KMS_HELPER) and helpful as
an indication what went wrong if we run into the same problem with a new
driver, instead of the cryptic link failure you get something like

WARNING: unmet direct dependencies detected for DRM_MIPI_DBI
  Depends on [m]: HAS_IOMEM [=y] && DRM_KMS_HELPER [=m]
  Selected by [y]:
  - DRM_PANEL_WIDECHIPS_WS2401 [=y] && HAS_IOMEM [=y] && DRM [=y] &&
DRM_PANEL [=y] && SPI [=y] && GPIOLIB [=y] && BACKLIGHT_CLASS_DEVICE
[=y]
  Selected by [m]:
  - TINYDRM_ILI9225 [=m] && HAS_IOMEM [=y] && DRM [=y] && SPI [=y]

> [1]
> https://lore.kernel.org/dri-devel/20220315084559.23510-1-tzimmermann@suse.de/

I was going for 'depends on' in the panel drivers because I saw the same being
done for other panel drivers, and mixing the two methods causes dependency
loops. I looked again now, and find that 'select DRM_KMS_HELPER' is more
common for other drivers, and makes sense here because it is generally
not user-selectable.

The easiest replacement for my patch would then be to just use 'select
DRM_KMS_HELPER' from CONFIG_DRM_MIPI_DBI, which makes it
safer and more consistent with your change. If you like, I'll send an updated
version.

One thing I'm not sure about is whether there is still use for ever having
CONFIG_DRM without CONFIG_DRM_KMS_HELPER if it gets selected
by almost every driver anyway. Is this actually a configuration that
users rely on, or should we just remove the symbol completely and
build the KMS helpers unconditionally?

       Arnd

Re: [PATCH] drm/panel: add CONFIG_DRM_KMS_HELPER dependencies

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 4105 bytes --]

Hi Arnd

Am 16.03.22 um 21:59 schrieb Arnd Bergmann:
> On Wed, Mar 16, 2022 at 8:31 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>> Am 16.03.22 um 20:12 schrieb Thomas Zimmermann:
>>>>
>>>> Adding a dependency in all drivers that select DRM_MIPI_DBI avoids
>>>> the problem for now, adding the dependency in DRM_MIPI_DBI as well
>>>> should help make it easier to figure out why it breaks if someone
>>>> forgets the dependency the next time.
>>>>
>>>>        tristate
>>>> -    depends on DRM
>>>> +    depends on DRM_KMS_HELPER
>>>
>>> This symbol cannot be selected by users, so it's maybe not a good idea
>>> to depend on it. In fact, I've had to remove such a statement because it
>>> created a cyclic dependency. [1]
> 
> I tried to explain above what I was thinking here: the added dependency
> is both a correct statement (DRM_MIPI_DBI depends on DRM_KMS_HELPER
> because it cannot be built without DRM_KMS_HELPER) and helpful as
> an indication what went wrong if we run into the same problem with a new
> driver, instead of the cryptic link failure you get something like
> 
> WARNING: unmet direct dependencies detected for DRM_MIPI_DBI
>    Depends on [m]: HAS_IOMEM [=y] && DRM_KMS_HELPER [=m]
>    Selected by [y]:
>    - DRM_PANEL_WIDECHIPS_WS2401 [=y] && HAS_IOMEM [=y] && DRM [=y] &&
> DRM_PANEL [=y] && SPI [=y] && GPIOLIB [=y] && BACKLIGHT_CLASS_DEVICE
> [=y]
>    Selected by [m]:
>    - TINYDRM_ILI9225 [=m] && HAS_IOMEM [=y] && DRM [=y] && SPI [=y]
> 
>> [1]
>> https://lore.kernel.org/dri-devel/20220315084559.23510-1-tzimmermann@suse.de/
> 
> I was going for 'depends on' in the panel drivers because I saw the same being
> done for other panel drivers, and mixing the two methods causes dependency
> loops. I looked again now, and find that 'select DRM_KMS_HELPER' is more
> common for other drivers, and makes sense here because it is generally
> not user-selectable.
> 
> The easiest replacement for my patch would then be to just use 'select
> DRM_KMS_HELPER' from CONFIG_DRM_MIPI_DBI, which makes it
> safer and more consistent with your change. If you like, I'll send an updated
> version.

MIPI DBI is another helper and select is not transitive IIRC. So drivers 
would still have to select KMS helpers as well. (?)

I just added my patch to drm-misc-fixes today and it should show up in 
upstream soon. Maybe just rebase your patch on top of it and if nothing 
breaks let's merge it as-is including the 'depends on'. You can add

Acked-by: Thomas Zimmermann <tzimmermann@suse.de>

in this case.

More generally, I think you're right about making DRM helper libraries 
using 'depends on' to link to other libraries. Drivers would at least 
know which config symbols to select. A number of config rules would have 
to be adapted to make that happen, I guess.

One issue is that different submodules of DRM seem to use different 
logic for expressing such config dependencies. That's been an endless 
source of problems so far.

> 
> One thing I'm not sure about is whether there is still use for ever having
> CONFIG_DRM without CONFIG_DRM_KMS_HELPER if it gets selected
> by almost every driver anyway. Is this actually a configuration that
> users rely on, or should we just remove the symbol completely and
> build the KMS helpers unconditionally?

Best leave it as it is. i915 doesn't use it. And since it's a helper, it 
should not be lumped together with core DRM code simply for reasons of 
design.

For DRM_KMS_HELPER itself, the mid-term plan is to move some of the code 
into other modules. KMS helpers used to contain all kind of helpers, but 
recently there's interest in reducing the minimum size of a built-in DRM 
with minimal driver support. So the non-essential stuff needs to go into 
modules for the more-sophisticated DRM drivers.

Best regards
Thomas

> 
>         Arnd

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH] drm/panel: add CONFIG_DRM_KMS_HELPER dependencies

[Arnd Bergmann]

On Thu, Mar 17, 2022 at 8:15 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
> Am 16.03.22 um 21:59 schrieb Arnd Bergmann:
> > On Wed, Mar 16, 2022 at 8:31 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
> >
> > I was going for 'depends on' in the panel drivers because I saw the same being
> > done for other panel drivers, and mixing the two methods causes dependency
> > loops. I looked again now, and find that 'select DRM_KMS_HELPER' is more
> > common for other drivers, and makes sense here because it is generally
> > not user-selectable.
> >
> > The easiest replacement for my patch would then be to just use 'select
> > DRM_KMS_HELPER' from CONFIG_DRM_MIPI_DBI, which makes it
> > safer and more consistent with your change. If you like, I'll send an updated
> > version.
>
> MIPI DBI is another helper and select is not transitive IIRC. So drivers
> would still have to select KMS helpers as well. (?)

Not sure what you mean here: if a driver selects DRM_MIPI_DBI,
and DRM_MIPI_DBI selects DRM_KMS_HELPER, the leaf driver
does not need to select DRM_KMS_HELPER because it is already
selected. This is one of the major problems of overusing 'select' because
you end up unable to turn things off.

Maybe you are thinking of the case where DRM_MIPI_DBI depends
on DRM_KMS_HELPER, and something selects DRM_MIPI_DBI.
In this case, the dependency does /not/ get inherited by the leaf
driver, it needs a copy of the dependency or it triggers a warning,
which is what my patch intended.

> More generally, I think you're right about making DRM helper libraries
> using 'depends on' to link to other libraries. Drivers would at least
> know which config symbols to select. A number of config rules would have
> to be adapted to make that happen, I guess.

Generally speaking, a problem with DRM is that it uses way too
much 'select' to enforce other subsystems to be enabled, this is
what causes DRM to have more problems with incorrect or circular
dependencies, and the only way to avoid that is to be consistent
about the dependencies: each symbol should only be referenced
with either 'select' or 'depends on' but not both, and 'select' should
ideally only be used on hidden symbols.

> > One thing I'm not sure about is whether there is still use for ever having
> > CONFIG_DRM without CONFIG_DRM_KMS_HELPER if it gets selected
> > by almost every driver anyway. Is this actually a configuration that
> > users rely on, or should we just remove the symbol completely and
> > build the KMS helpers unconditionally?
>
> Best leave it as it is. i915 doesn't use it. And since it's a helper, it
> should not be lumped together with core DRM code simply for reasons of
> design.

Ok

> For DRM_KMS_HELPER itself, the mid-term plan is to move some of the code
> into other modules. KMS helpers used to contain all kind of helpers, but
> recently there's interest in reducing the minimum size of a built-in DRM
> with minimal driver support. So the non-essential stuff needs to go into
> modules for the more-sophisticated DRM drivers.

Right, that makes sense.

       Arnd