Re: [PATCH v1 2/2] drivers/gpu/drm: don't select DMA_CMA or CMA from aspeed or etnaviv

[Arnd Bergmann]

On Thu, Apr 8, 2021 at 2:00 PM David Hildenbrand <david@xxxxxxxxxx> wrote:
>
> On 08.04.21 13:44, Arnd Bergmann wrote:
> > On Thu, Apr 8, 2021 at 1:00 PM David Hildenbrand <david@xxxxxxxxxx> 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
> >
> >           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.

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