Trying to set CONFIG_CMA=y with CONFIG_DMA_CMA=n revealed that we have three drivers that select these options. Random drivers should not override user settings of such core knobs. Let's use "imply DMA_CMA" instead, such that user configuration and dependencies are respected.
Cc: Joel Stanley joel@jms.id.au Cc: David Airlie airlied@linux.ie Cc: Daniel Vetter daniel@ffwll.ch Cc: Andrew Jeffery andrew@aj.id.au Cc: Lucas Stach l.stach@pengutronix.de Cc: Russell King linux+etnaviv@armlinux.org.uk Cc: Christian Gmeiner christian.gmeiner@gmail.com Cc: Mike Rapoport rppt@kernel.org Cc: Arnd Bergmann arnd@arndb.de Cc: Bartlomiej Zolnierkiewicz b.zolnierkie@samsung.com Cc: Linus Walleij linus.walleij@linaro.org Cc: Michal Simek michal.simek@xilinx.com Cc: Masahiro Yamada masahiroy@kernel.org Cc: Randy Dunlap rdunlap@infradead.org Cc: Peter Collingbourne pcc@google.com Cc: linux-aspeed@lists.ozlabs.org Cc: dri-devel@lists.freedesktop.org Cc: linux-arm-kernel@lists.infradead.org Cc: etnaviv@lists.freedesktop.org Cc: linux-fbdev@vger.kernel.org
David Hildenbrand (2): drivers/video/fbdev: don't select DMA_CMA drivers/gpu/drm: don't select DMA_CMA or CMA from aspeed or etnaviv
drivers/gpu/drm/aspeed/Kconfig | 3 +-- drivers/gpu/drm/etnaviv/Kconfig | 3 +-- drivers/video/fbdev/Kconfig | 2 +- 3 files changed, 3 insertions(+), 5 deletions(-)
Random drivers should not override a user configuration of core knobs (e.g., CONFIG_DMA_CMA=n). Use "imply" instead, to still respect dependencies and manual overrides.
"This is similar to "select" as it enforces a lower limit on another symbol except that the "implied" symbol's value may still be set to n from a direct dependency or with a visible prompt."
Implying DRM_CMA should be sufficient, as that depends on CMA.
Note: If this is a real dependency, we should use "depends on DMA_CMA" instead - but I assume the driver can work without CMA just fine -- esp. when we wouldn't have HAVE_DMA_CONTIGUOUS or CMA right now.
Signed-off-by: David Hildenbrand david@redhat.com --- drivers/video/fbdev/Kconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/video/fbdev/Kconfig b/drivers/video/fbdev/Kconfig index 4f02db65dede..d37cd5341e1b 100644 --- a/drivers/video/fbdev/Kconfig +++ b/drivers/video/fbdev/Kconfig @@ -2186,7 +2186,7 @@ config FB_HYPERV select FB_CFB_COPYAREA select FB_CFB_IMAGEBLIT select FB_DEFERRED_IO - select DMA_CMA if HAVE_DMA_CONTIGUOUS && CMA + imply DMA_CMA help This framebuffer driver supports Microsoft Hyper-V Synthetic Video.
On Thu, Apr 08, 2021 at 11:20:10AM +0200, David Hildenbrand wrote:
Random drivers should not override a user configuration of core knobs (e.g., CONFIG_DMA_CMA=n). Use "imply" instead, to still respect dependencies and manual overrides.
"This is similar to "select" as it enforces a lower limit on another symbol except that the "implied" symbol's value may still be set to n from a direct dependency or with a visible prompt."
Implying DRM_CMA should be sufficient, as that depends on CMA.
^ DMA_CMA ?
Note: If this is a real dependency, we should use "depends on DMA_CMA" instead - but I assume the driver can work without CMA just fine -- esp. when we wouldn't have HAVE_DMA_CONTIGUOUS or CMA right now.
Signed-off-by: David Hildenbrand david@redhat.com
drivers/video/fbdev/Kconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/video/fbdev/Kconfig b/drivers/video/fbdev/Kconfig index 4f02db65dede..d37cd5341e1b 100644 --- a/drivers/video/fbdev/Kconfig +++ b/drivers/video/fbdev/Kconfig @@ -2186,7 +2186,7 @@ config FB_HYPERV select FB_CFB_COPYAREA select FB_CFB_IMAGEBLIT select FB_DEFERRED_IO
- select DMA_CMA if HAVE_DMA_CONTIGUOUS && CMA
- imply DMA_CMA help This framebuffer driver supports Microsoft Hyper-V Synthetic Video.
-- 2.30.2
Random drivers should not override a user configuration of core knobs (e.g., CONFIG_DMA_CMA=n). Use "imply" instead, to still respect dependencies and manual overrides.
"This is similar to "select" as it enforces a lower limit on another symbol except that the "implied" symbol's value may still be set to n from a direct dependency or with a visible prompt."
Implying DRM_CMA should be sufficient, as that depends on CMA.
Note: If this is a real dependency, we should use "depends on DMA_CMA" instead - but I assume the driver can work without CMA just fine -- esp. when we wouldn't have HAVE_DMA_CONTIGUOUS right now.
Signed-off-by: David Hildenbrand david@redhat.com --- drivers/gpu/drm/aspeed/Kconfig | 3 +-- drivers/gpu/drm/etnaviv/Kconfig | 3 +-- 2 files changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/aspeed/Kconfig b/drivers/gpu/drm/aspeed/Kconfig index 5e95bcea43e9..a055f763d230 100644 --- a/drivers/gpu/drm/aspeed/Kconfig +++ b/drivers/gpu/drm/aspeed/Kconfig @@ -6,9 +6,8 @@ config DRM_ASPEED_GFX depends on MMU select DRM_KMS_HELPER select DRM_KMS_CMA_HELPER - select DMA_CMA if HAVE_DMA_CONTIGUOUS - select CMA if HAVE_DMA_CONTIGUOUS select MFD_SYSCON + imply DRM_CMA help Chose this option if you have an ASPEED AST2500 SOC Display Controller (aka GFX). diff --git a/drivers/gpu/drm/etnaviv/Kconfig b/drivers/gpu/drm/etnaviv/Kconfig index faa7fc68b009..5f5576b7221a 100644 --- a/drivers/gpu/drm/etnaviv/Kconfig +++ b/drivers/gpu/drm/etnaviv/Kconfig @@ -9,9 +9,8 @@ config DRM_ETNAVIV select THERMAL if DRM_ETNAVIV_THERMAL select TMPFS select WANT_DEV_COREDUMP - select CMA if HAVE_DMA_CONTIGUOUS - select DMA_CMA if HAVE_DMA_CONTIGUOUS select DRM_SCHED + imply DMA_CMA help DRM driver for Vivante GPUs.
On Thu, Apr 08, 2021 at 11:20:11AM +0200, David Hildenbrand wrote:
Random drivers should not override a user configuration of core knobs (e.g., CONFIG_DMA_CMA=n). Use "imply" instead, to still respect dependencies and manual overrides.
"This is similar to "select" as it enforces a lower limit on another symbol except that the "implied" symbol's value may still be set to n from a direct dependency or with a visible prompt."
Implying DRM_CMA should be sufficient, as that depends on CMA.
^ DMA_CMA
Note: If this is a real dependency, we should use "depends on DMA_CMA" instead - but I assume the driver can work without CMA just fine -- esp. when we wouldn't have HAVE_DMA_CONTIGUOUS right now.
Signed-off-by: David Hildenbrand david@redhat.com
drivers/gpu/drm/aspeed/Kconfig | 3 +-- drivers/gpu/drm/etnaviv/Kconfig | 3 +-- 2 files changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/aspeed/Kconfig b/drivers/gpu/drm/aspeed/Kconfig index 5e95bcea43e9..a055f763d230 100644 --- a/drivers/gpu/drm/aspeed/Kconfig +++ b/drivers/gpu/drm/aspeed/Kconfig @@ -6,9 +6,8 @@ config DRM_ASPEED_GFX depends on MMU select DRM_KMS_HELPER select DRM_KMS_CMA_HELPER
- select DMA_CMA if HAVE_DMA_CONTIGUOUS
- select CMA if HAVE_DMA_CONTIGUOUS select MFD_SYSCON
- imply DRM_CMA
Ditto
help Chose this option if you have an ASPEED AST2500 SOC Display Controller (aka GFX). diff --git a/drivers/gpu/drm/etnaviv/Kconfig b/drivers/gpu/drm/etnaviv/Kconfig index faa7fc68b009..5f5576b7221a 100644 --- a/drivers/gpu/drm/etnaviv/Kconfig +++ b/drivers/gpu/drm/etnaviv/Kconfig @@ -9,9 +9,8 @@ config DRM_ETNAVIV select THERMAL if DRM_ETNAVIV_THERMAL select TMPFS select WANT_DEV_COREDUMP
- select CMA if HAVE_DMA_CONTIGUOUS
- select DMA_CMA if HAVE_DMA_CONTIGUOUS select DRM_SCHED
- imply DMA_CMA help DRM driver for Vivante GPUs.
-- 2.30.2
On 08.04.21 11:56, Mike Rapoport wrote:
On Thu, Apr 08, 2021 at 11:20:11AM +0200, David Hildenbrand wrote:
Random drivers should not override a user configuration of core knobs (e.g., CONFIG_DMA_CMA=n). Use "imply" instead, to still respect dependencies and manual overrides.
"This is similar to "select" as it enforces a lower limit on another symbol except that the "implied" symbol's value may still be set to n from a direct dependency or with a visible prompt."
Implying DRM_CMA should be sufficient, as that depends on CMA.
^ DMA_CMANote: If this is a real dependency, we should use "depends on DMA_CMA" instead - but I assume the driver can work without CMA just fine -- esp. when we wouldn't have HAVE_DMA_CONTIGUOUS right now.
Signed-off-by: David Hildenbrand david@redhat.com
drivers/gpu/drm/aspeed/Kconfig | 3 +-- drivers/gpu/drm/etnaviv/Kconfig | 3 +-- 2 files changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/aspeed/Kconfig b/drivers/gpu/drm/aspeed/Kconfig index 5e95bcea43e9..a055f763d230 100644 --- a/drivers/gpu/drm/aspeed/Kconfig +++ b/drivers/gpu/drm/aspeed/Kconfig @@ -6,9 +6,8 @@ config DRM_ASPEED_GFX depends on MMU select DRM_KMS_HELPER select DRM_KMS_CMA_HELPER
- select DMA_CMA if HAVE_DMA_CONTIGUOUS
- select CMA if HAVE_DMA_CONTIGUOUS select MFD_SYSCON
- imply DRM_CMA
Ditto
Gah, thanks!
On Thu, Apr 8, 2021 at 11:22 AM David Hildenbrand david@redhat.com wrote:
Random drivers should not override a user configuration of core knobs (e.g., CONFIG_DMA_CMA=n). Use "imply" instead, to still respect dependencies and manual overrides.
"This is similar to "select" as it enforces a lower limit on another symbol except that the "implied" symbol's value may still be set to n from a direct dependency or with a visible prompt."
Implying DRM_CMA should be sufficient, as that depends on CMA.
Note: If this is a real dependency, we should use "depends on DMA_CMA" instead - but I assume the driver can work without CMA just fine -- esp. when we wouldn't have HAVE_DMA_CONTIGUOUS right now.
'imply' is almost never the right solution, and it tends to cause more problems than it solves.
In particular, it does not prevent a configuration with 'DRM_CMA=m' and 'DRMA_ASPEED_GFX=y', or any build failures from such a configuration.
If you want this kind of soft dependency, you need 'depends on DRM_CMA || !DRM_CMA'.
Arnd
On 08.04.21 12:20, Arnd Bergmann wrote:
On Thu, Apr 8, 2021 at 11:22 AM David Hildenbrand david@redhat.com wrote:
Random drivers should not override a user configuration of core knobs (e.g., CONFIG_DMA_CMA=n). Use "imply" instead, to still respect dependencies and manual overrides.
"This is similar to "select" as it enforces a lower limit on another symbol except that the "implied" symbol's value may still be set to n from a direct dependency or with a visible prompt."
Implying DRM_CMA should be sufficient, as that depends on CMA.
Note: If this is a real dependency, we should use "depends on DMA_CMA" instead - but I assume the driver can work without CMA just fine -- esp. when we wouldn't have HAVE_DMA_CONTIGUOUS right now.
'imply' is almost never the right solution, and it tends to cause more problems than it solves.
I thought that was the case with "select" :)
In particular, it does not prevent a configuration with 'DRM_CMA=m'
I assume you meant "DRM_CMA=n" ? DRM_CMA cannot be built as a module.
and 'DRMA_ASPEED_GFX=y', or any build failures from such a configuration.
I don't follow. "DRM_CMA=n" and 'DRMA_ASPEED_GFX=y' is supposed to work just fine (e.g., without HAVE_DMA_CONTIGUOUS) or what am I missing?
If you want this kind of soft dependency, you need 'depends on DRM_CMA || !DRM_CMA'.
Seriously? I think the point of imply is "please enable if possible and not prevented by someone else". Your example looks more like a NOP - no? Or will it have the same effect?
On 08.04.21 12:27, David Hildenbrand wrote:
On 08.04.21 12:20, Arnd Bergmann wrote:
On Thu, Apr 8, 2021 at 11:22 AM David Hildenbrand david@redhat.com wrote:
Random drivers should not override a user configuration of core knobs (e.g., CONFIG_DMA_CMA=n). Use "imply" instead, to still respect dependencies and manual overrides.
"This is similar to "select" as it enforces a lower limit on another symbol except that the "implied" symbol's value may still be set to n from a direct dependency or with a visible prompt."
Implying DRM_CMA should be sufficient, as that depends on CMA.
Note: If this is a real dependency, we should use "depends on DMA_CMA" instead - but I assume the driver can work without CMA just fine -- esp. when we wouldn't have HAVE_DMA_CONTIGUOUS right now.
'imply' is almost never the right solution, and it tends to cause more problems than it solves.
I thought that was the case with "select" :)
In particular, it does not prevent a configuration with 'DRM_CMA=m'
I assume you meant "DRM_CMA=n" ? DRM_CMA cannot be built as a module.
and 'DRMA_ASPEED_GFX=y', or any build failures from such a configuration.
I don't follow. "DRM_CMA=n" and 'DRMA_ASPEED_GFX=y' is supposed to work just fine (e.g., without HAVE_DMA_CONTIGUOUS) or what am I missing?
If you want this kind of soft dependency, you need 'depends on DRM_CMA || !DRM_CMA'.
Seriously? I think the point of imply is "please enable if possible and not prevented by someone else". Your example looks more like a NOP - no? Or will it have the same effect?
I just tried (remove CONFIG_DMA_CMA from .config followed by make) and the default will be set to "N" (when querying the user). So it indeed looks like a NOP - unless I am missing something.
On Thu, Apr 8, 2021 at 12:29 PM David Hildenbrand david@redhat.com wrote:
On 08.04.21 12:20, Arnd Bergmann wrote:
On Thu, Apr 8, 2021 at 11:22 AM David Hildenbrand david@redhat.com wrote:
Random drivers should not override a user configuration of core knobs (e.g., CONFIG_DMA_CMA=n). Use "imply" instead, to still respect dependencies and manual overrides.
"This is similar to "select" as it enforces a lower limit on another symbol except that the "implied" symbol's value may still be set to n from a direct dependency or with a visible prompt."
Implying DRM_CMA should be sufficient, as that depends on CMA.
Note: If this is a real dependency, we should use "depends on DMA_CMA" instead - but I assume the driver can work without CMA just fine -- esp. when we wouldn't have HAVE_DMA_CONTIGUOUS right now.
'imply' is almost never the right solution, and it tends to cause more problems than it solves.
I thought that was the case with "select" :)
Yes, but that's a different set of problems
In particular, it does not prevent a configuration with 'DRM_CMA=m'
I assume you meant "DRM_CMA=n" ? DRM_CMA cannot be built as a module.
Ok, at least that makes it easier.
and 'DRMA_ASPEED_GFX=y', or any build failures from such a configuration.
I don't follow. "DRM_CMA=n" and 'DRMA_ASPEED_GFX=y' is supposed to work just fine (e.g., without HAVE_DMA_CONTIGUOUS) or what am I missing?
I thought you were trying to solve the problem where DRMA_ASPEED_GFX can optionally link against CMA but would fail to build when the CMA code is in a loadable module.
If the problem you are trying to solve is a different one, you need a different solution, not what I posted above.
If you want this kind of soft dependency, you need 'depends on DRM_CMA || !DRM_CMA'.
Seriously? I think the point of imply is "please enable if possible and not prevented by someone else".
That used to be the meaning, but it changed a few years ago. Now it means "when a used manually turns on this symbol, turn on the implied one as well, but let them turn it off again if they choose".
This is pretty much a NOP.
Your example looks more like a NOP - no? Or will it have the same effect?
The example I gave is only meaningful if both are tristate, which is not the case here as you explain.
It is a somewhat awkward way to say "prevent this symbol from being =y if the dependency is =m".
Arnd
In particular, it does not prevent a configuration with 'DRM_CMA=m'
I assume you meant "DRM_CMA=n" ? DRM_CMA cannot be built as a module.
Ok, at least that makes it easier.
and 'DRMA_ASPEED_GFX=y', or any build failures from such a configuration.
I don't follow. "DRM_CMA=n" and 'DRMA_ASPEED_GFX=y' is supposed to work just fine (e.g., without HAVE_DMA_CONTIGUOUS) or what am I missing?
I thought you were trying to solve the problem where DRMA_ASPEED_GFX can optionally link against CMA but would fail to build when the CMA code is in a loadable module.
Yes. I was trying to say: it works with this patch just fine. The issue you described does not seem to apply (DRM_CMA=m).
Your example looks more like a NOP - no? Or will it have the same effect?
The example I gave is only meaningful if both are tristate, which is not the case here as you explain.
Okay, thanks.
It is a somewhat awkward way to say "prevent this symbol from being =y if the dependency is =m".
What would be the right thing to do in the case here then to achieve the "if DRMA_ASPEED_GFX is enabled, also enable DMA_CMA id possible"?
One approach could be to have for DMA_CMA
default y if DRMA_ASPEED_GFX
but it feels like the wrong way to tackle this.
Thanks!
On Thu, Apr 8, 2021 at 1:00 PM David Hildenbrand david@redhat.com wrote:
It is a somewhat awkward way to say "prevent this symbol from being =y if the dependency is =m".
What would be the right thing to do in the case here then to achieve the "if DRMA_ASPEED_GFX is enabled, also enable DMA_CMA id possible"?
One approach could be to have for DMA_CMA
default y if DRMA_ASPEED_GFX
but it feels like the wrong way to tackle this.
I'm still not sure what you are trying to achieve. Is the idea only to provide a useful default for DMA_CMA depending on which drivers are enabled?
This is something you could do using a hidden helper symbol like
config DRMA_ASPEED_GFX bool "Aspeed display driver" select DRM_WANT_CMA
config DRM_WANT_CMA bool help Select this from any driver that benefits from CMA being enabled
config DMA_CMA bool "Use CMA helpers for DRM" default DRM_WANT_CMA
Arnd
On 08.04.21 13:44, Arnd Bergmann wrote:
On Thu, Apr 8, 2021 at 1:00 PM David Hildenbrand david@redhat.com wrote:
It is a somewhat awkward way to say "prevent this symbol from being =y if the dependency is =m".
What would be the right thing to do in the case here then to achieve the "if DRMA_ASPEED_GFX is enabled, also enable DMA_CMA id possible"?
One approach could be to have for DMA_CMA
default y if DRMA_ASPEED_GFX
but it feels like the wrong way to tackle this.
I'm still not sure what you are trying to achieve. Is the idea only to provide a useful default for DMA_CMA depending on which drivers are enabled?
"Random drivers should not override a user configuration of core knobs (e.g., CONFIG_DMA_CMA=n)."
Let's assume I'm a distribution and want to set CONFIG_CMA=n or want to set CONFIG_DMA_CMA=n with CONFIG_CMA=y; there is no way to do that with e.g., DRMA_ASPEED_GFX=y because it will always override my (user!) setting -- even though it doesn't really always need it. Using "select" is the problem here.
This is something you could do using a hidden helper symbol like
config DRMA_ASPEED_GFX bool "Aspeed display driver" select DRM_WANT_CMA
config DRM_WANT_CMA bool help Select this from any driver that benefits from CMA being enabled
config DMA_CMA bool "Use CMA helpers for DRM" default DRM_WANT_CMA
Arnd
That's precisely what I had first, with an additional "WANT_CMA" -- but looking at the number of such existing options (I was able to spot 1 !) I wondered if there is a better approach to achieve the same; "imply" sounded like a good candidate.
On Thu, Apr 8, 2021 at 2:00 PM David Hildenbrand david@redhat.com wrote:
On 08.04.21 13:44, Arnd Bergmann wrote:
On Thu, Apr 8, 2021 at 1:00 PM David Hildenbrand david@redhat.com wrote:
It is a somewhat awkward way to say "prevent this symbol from being =y if the dependency is =m".
What would be the right thing to do in the case here then to achieve the "if DRMA_ASPEED_GFX is enabled, also enable DMA_CMA id possible"?
One approach could be to have for DMA_CMA
default y if DRMA_ASPEED_GFX
but it feels like the wrong way to tackle this.
I'm still not sure what you are trying to achieve. Is the idea only to provide a useful default for DMA_CMA depending on which drivers are enabled?
"Random drivers should not override a user configuration of core knobs (e.g., CONFIG_DMA_CMA=n)."
Let's assume I'm a distribution and want to set CONFIG_CMA=n or want to set CONFIG_DMA_CMA=n with CONFIG_CMA=y; there is no way to do that with e.g., DRMA_ASPEED_GFX=y because it will always override my (user!) setting -- even though it doesn't really always need it. Using "select" is the problem here.
I agree on the part of removing the 'select' if we don't need it. The part I couldn't figure out was what the 'imply' is supposed to help with. Most other users that added imply tried (and failed) to fix a build problem.
This is something you could do using a hidden helper symbol like
config DRMA_ASPEED_GFX bool "Aspeed display driver" select DRM_WANT_CMA
config DRM_WANT_CMA bool help Select this from any driver that benefits from CMA being enabled
config DMA_CMA bool "Use CMA helpers for DRM" default DRM_WANT_CMA
ArndThat's precisely what I had first, with an additional "WANT_CMA" -- but looking at the number of such existing options (I was able to spot 1 !) I wondered if there is a better approach to achieve the same; "imply" sounded like a good candidate.
I can probably find a couple more, but regardless of how many others exist, this would be a much clearer way of doing it than 'imply' since it has none of the ambiguity and misuse problems.
I think the reason we don't see more is that generally speaking, those defaults are widely ignored anyway. You almost always start out with a defconfig file that contains everything you know you need, and then you add bits to that. Having the default in any form only helps to make that defconfig file one line shorter, while requiring other users to add another line to turn it off when they do not want it.
Arnd
On Thu, Apr 8, 2021 at 2:01 PM David Hildenbrand david@redhat.com wrote:
This is something you could do using a hidden helper symbol like
config DRMA_ASPEED_GFX bool "Aspeed display driver" select DRM_WANT_CMA
config DRM_WANT_CMA bool help Select this from any driver that benefits from CMA being enabled
config DMA_CMA bool "Use CMA helpers for DRM" default DRM_WANT_CMA
ArndThat's precisely what I had first, with an additional "WANT_CMA" -- but looking at the number of such existing options (I was able to spot 1 !)
If you do this it probably makes sense to fix a few other drivers Kconfig in the process. It's not just a problem with your driver. "my" drivers:
drivers/gpu/drm/mcde/Kconfig drivers/gpu/drm/pl111/Kconfig drivers/gpu/drm/tve200/Kconfig
certainly needs this as well, and pretty much anything that is selecting DRM_KMS_CMA_HELPER or DRM_GEM_CMA_HELPER "wants" DMA_CMA.
Yours, Linus Walleij
On Thu, Apr 8, 2021 at 2:50 PM Linus Walleij linus.walleij@linaro.org wrote:
On Thu, Apr 8, 2021 at 2:01 PM David Hildenbrand david@redhat.com wrote:
This is something you could do using a hidden helper symbol like
config DRMA_ASPEED_GFX bool "Aspeed display driver" select DRM_WANT_CMA
config DRM_WANT_CMA bool help Select this from any driver that benefits from CMA being enabled
config DMA_CMA bool "Use CMA helpers for DRM" default DRM_WANT_CMA
ArndThat's precisely what I had first, with an additional "WANT_CMA" -- but looking at the number of such existing options (I was able to spot 1 !)
If you do this it probably makes sense to fix a few other drivers Kconfig in the process. It's not just a problem with your driver. "my" drivers:
drivers/gpu/drm/mcde/Kconfig drivers/gpu/drm/pl111/Kconfig drivers/gpu/drm/tve200/Kconfig
certainly needs this as well, and pretty much anything that is selecting DRM_KMS_CMA_HELPER or DRM_GEM_CMA_HELPER "wants" DMA_CMA.
Are there any that don't select either of the helpers and still want CMA? If not, it would be easy to just add
default DRM_KMS_CMA_HELPER || DRM_GEM_CMA_HELPER
and skipt the extra symbol.
Arnd
On 08.04.21 15:19, Arnd Bergmann wrote:
On Thu, Apr 8, 2021 at 2:50 PM Linus Walleij linus.walleij@linaro.org wrote:
On Thu, Apr 8, 2021 at 2:01 PM David Hildenbrand david@redhat.com wrote:
This is something you could do using a hidden helper symbol like
config DRMA_ASPEED_GFX bool "Aspeed display driver" select DRM_WANT_CMA
config DRM_WANT_CMA bool help Select this from any driver that benefits from CMA being enabled
config DMA_CMA bool "Use CMA helpers for DRM" default DRM_WANT_CMA
ArndThat's precisely what I had first, with an additional "WANT_CMA" -- but looking at the number of such existing options (I was able to spot 1 !)
If you do this it probably makes sense to fix a few other drivers Kconfig in the process. It's not just a problem with your driver. "my" drivers:
drivers/gpu/drm/mcde/Kconfig drivers/gpu/drm/pl111/Kconfig drivers/gpu/drm/tve200/Kconfig
certainly needs this as well, and pretty much anything that is selecting DRM_KMS_CMA_HELPER or DRM_GEM_CMA_HELPER "wants" DMA_CMA.
Are there any that don't select either of the helpers and still want CMA? If not, it would be easy to just add
default DRM_KMS_CMA_HELPER || DRM_GEM_CMA_HELPERand skipt the extra symbol.
That sounds like a reasonable thing to do. I'll look into that.
On 08.04.21 14:49, Linus Walleij wrote:
On Thu, Apr 8, 2021 at 2:01 PM David Hildenbrand david@redhat.com wrote:
This is something you could do using a hidden helper symbol like
config DRMA_ASPEED_GFX bool "Aspeed display driver" select DRM_WANT_CMA
config DRM_WANT_CMA bool help Select this from any driver that benefits from CMA being enabled
config DMA_CMA bool "Use CMA helpers for DRM" default DRM_WANT_CMA
ArndThat's precisely what I had first, with an additional "WANT_CMA" -- but looking at the number of such existing options (I was able to spot 1 !)
If you do this it probably makes sense to fix a few other drivers Kconfig in the process. It's not just a problem with your driver. "my" drivers:
:) I actually wanted to convert them to "depends on DMA_CMA" but ran into recursive dependencies ...
drivers/gpu/drm/mcde/Kconfig drivers/gpu/drm/pl111/Kconfig drivers/gpu/drm/tve200/Kconfig
I was assuming these are "real" dependencies. Will it also work without DMA_CMA?
certainly needs this as well, and pretty much anything that is selecting DRM_KMS_CMA_HELPER or DRM_GEM_CMA_HELPER "wants" DMA_CMA.
"wants" as in "desires to use but can life without" or "wants" as in "really needs it". ?
On Thu, Apr 8, 2021 at 6:45 PM David Hildenbrand david@redhat.com wrote:
On 08.04.21 14:49, Linus Walleij wrote:
On Thu, Apr 8, 2021 at 2:01 PM David Hildenbrand david@redhat.com wrote:
This is something you could do using a hidden helper symbol like
config DRMA_ASPEED_GFX bool "Aspeed display driver" select DRM_WANT_CMA
config DRM_WANT_CMA bool help Select this from any driver that benefits from CMA being enabled
config DMA_CMA bool "Use CMA helpers for DRM" default DRM_WANT_CMA
ArndThat's precisely what I had first, with an additional "WANT_CMA" -- but looking at the number of such existing options (I was able to spot 1 !)
If you do this it probably makes sense to fix a few other drivers Kconfig in the process. It's not just a problem with your driver. "my" drivers:
:) I actually wanted to convert them to "depends on DMA_CMA" but ran into recursive dependencies ...
drivers/gpu/drm/mcde/Kconfig drivers/gpu/drm/pl111/Kconfig drivers/gpu/drm/tve200/Kconfig
Right, this is the main problem caused by using 'select' to force-enable symbols that other drivers depend on.
Usually, the answer is to be consistent about the use of 'select' and 'depends on', using the former only to enable symbols that are hidden, while using 'depends on' for anything that is an actual build time dependency.
I was assuming these are "real" dependencies. Will it also work without DMA_CMA?
I think in this case, it is fairly likely to work without DMA_CMA when the probe function gets called during a fresh boot, but fairly likely to fail if it gets called after the system has run for long enough to fragment the free memory.
The point of DMA_CMA is to make it work reliably.
Arnd
On 08.04.21 22:29, Arnd Bergmann wrote:
On Thu, Apr 8, 2021 at 6:45 PM David Hildenbrand david@redhat.com wrote:
On 08.04.21 14:49, Linus Walleij wrote:
On Thu, Apr 8, 2021 at 2:01 PM David Hildenbrand david@redhat.com wrote:
This is something you could do using a hidden helper symbol like
config DRMA_ASPEED_GFX bool "Aspeed display driver" select DRM_WANT_CMA
config DRM_WANT_CMA bool help Select this from any driver that benefits from CMA being enabled
config DMA_CMA bool "Use CMA helpers for DRM" default DRM_WANT_CMA
ArndThat's precisely what I had first, with an additional "WANT_CMA" -- but looking at the number of such existing options (I was able to spot 1 !)
If you do this it probably makes sense to fix a few other drivers Kconfig in the process. It's not just a problem with your driver. "my" drivers:
:) I actually wanted to convert them to "depends on DMA_CMA" but ran into recursive dependencies ...
drivers/gpu/drm/mcde/Kconfig drivers/gpu/drm/pl111/Kconfig drivers/gpu/drm/tve200/Kconfig
Right, this is the main problem caused by using 'select' to force-enable symbols that other drivers depend on.
Usually, the answer is to be consistent about the use of 'select' and 'depends on', using the former only to enable symbols that are hidden, while using 'depends on' for anything that is an actual build time dependency.
I was assuming these are "real" dependencies. Will it also work without DMA_CMA?
I think in this case, it is fairly likely to work without DMA_CMA when the probe function gets called during a fresh boot, but fairly likely to fail if it gets called after the system has run for long enough to fragment the free memory.
The point of DMA_CMA is to make it work reliably.
Right, and even at runtime there is no guarantee that DMA_CMA will do anything -- especially if we don't reserve a CMA region (e.g., "cma=X").
So this really sounds like a
"desires DMA_CMA"
and achieving that via an additional symbol or via "default y if ..." for DMA_CMA sounds reasonable.
Thanks!
On Thu, Apr 8, 2021 at 6:44 PM David Hildenbrand david@redhat.com wrote:
drivers/gpu/drm/mcde/Kconfig drivers/gpu/drm/pl111/Kconfig drivers/gpu/drm/tve200/Kconfig
I was assuming these are "real" dependencies. Will it also work without DMA_CMA?
It will mostly work but that is only because the reservations are mostly contiguous anyway because they are done early and are small. The hardware requires contiguous buffers in all three cases. I'm not sure I always got it right.
certainly needs this as well, and pretty much anything that is selecting DRM_KMS_CMA_HELPER or DRM_GEM_CMA_HELPER "wants" DMA_CMA.
"wants" as in "desires to use but can life without" or "wants" as in "really needs it". ?
I don't know the exact semantics of using DRM_KMS_CMA* without actually using DMA_CMA. I suspect small allocations will be contiguous and big allocations will start to fragment? but it's just my guess. I guess "really need it"?
Yours, Linus Walleij
On Thu, Apr 08, 2021 at 12:20:50PM +0200, Arnd Bergmann wrote:
On Thu, Apr 8, 2021 at 11:22 AM David Hildenbrand david@redhat.com wrote:
Random drivers should not override a user configuration of core knobs (e.g., CONFIG_DMA_CMA=n). Use "imply" instead, to still respect dependencies and manual overrides.
"This is similar to "select" as it enforces a lower limit on another symbol except that the "implied" symbol's value may still be set to n from a direct dependency or with a visible prompt."
Implying DRM_CMA should be sufficient, as that depends on CMA.
Note: If this is a real dependency, we should use "depends on DMA_CMA" instead - but I assume the driver can work without CMA just fine -- esp. when we wouldn't have HAVE_DMA_CONTIGUOUS right now.
'imply' is almost never the right solution, and it tends to cause more problems than it solves.
In particular, it does not prevent a configuration with 'DRM_CMA=m' and 'DRMA_ASPEED_GFX=y', or any build failures from such a configuration.
If you want this kind of soft dependency, you need 'depends on DRM_CMA || !DRM_CMA'.
The problem is that depends on is a real pain for users to find their drivers. This is why we have a ton of select, because the kconfig ui tends to suck.
If you want to change this, we need automatic conflict resolution like apt and other package managers have, with suggestions how to fix the config if you want to enable a driver, but some of its requirements are missing. The current approach of hiding driver symbols complete if any of their dependencies are off is really not great. -Daniel
On Thu, Apr 08, 2021 at 01:38:17PM +0200, Daniel Vetter wrote:
If you want to change this, we need automatic conflict resolution like apt and other package managers have, with suggestions how to fix the config if you want to enable a driver, but some of its requirements are missing. The current approach of hiding driver symbols complete if any of their dependencies are off is really not great.
+1 to this..
Though Kconfig is basically already unusuable unless you have hours to carefully craft the kconfig you need to get out.
I'm not sure trying to optimize this by abusing the existing language rules is such a good idea.
I gave a very half hearted go at a simple heuristic solution solve for kconfig a while ago. It is good enough to sometimes automate a kconfig task, but it is not so nice.
I use it to do things like "turn on all RDMA drivers" which is quite a hard to do by hand.
It looks liked heursitics need a lot of fine tuning as the conditionals are complex enough that it is hard to guess which branch is going to yield a success.
Jason
dri-devel@lists.freedesktop.org
-
Arnd Bergmann -
Daniel Vetter -
David Hildenbrand -
Jason Gunthorpe -
Linus Walleij -
Mike Rapoport