Re: [PATCH] drm: aspeed: Select CMA only if available

[Joel Stanley <joel@jms.id.au>]

On Fri, 5 Apr 2019 at 11:20, Noralf Trønnes <noralf@tronnes.org> wrote:
>
>
>
> Den 05.04.2019 07.28, skrev Joel Stanley:
> > When building this driver for architectures where CMA is not available.
> >
> > Fixes: 4f2a8f5898ec ("drm: Add ASPEED GFX driver")
> > Reported-by: Stephen Rothwell <sfr@canb.auug.org.au>
> > Reported-by: kernel test robot <lkp@intel.com>
> > Signed-off-by: Joel Stanley <joel@jms.id.au>
> > ---
> > This fixes the build break.
> >
> > Another question is if we need to select this at all, as we have a
> > reserved memory region to allocate from. I am not familiar with this
> > area, so advice is welcome.
> >
> >  drivers/gpu/drm/aspeed/Kconfig | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/aspeed/Kconfig b/drivers/gpu/drm/aspeed/Kconfig
> > index 42b74d18a41b..1775fb85ec74 100644
> > --- a/drivers/gpu/drm/aspeed/Kconfig
> > +++ b/drivers/gpu/drm/aspeed/Kconfig
> > @@ -4,8 +4,8 @@ config DRM_ASPEED_GFX
> >       select DRM_KMS_HELPER
> >       select DRM_KMS_CMA_HELPER
> >       select DRM_PANEL
> > -     select DMA_CMA
> > -     select CMA
> > +     select DMA_CMA if HAVE_DMA_CONTIGUOUS
> > +     select CMA if HAVE_DMA_CONTIGUOUS
>
> I'm no expert on this, but the rule is that you should not select
> options that are user visible which both of these are. The user should
> select them. So I believe that you should remove them.
>
> Docs: Documentation/kbuild/kconfig-language.txt

I agree with your point, I was following what etnaviv did in this case.

As it's written the driver requires CMA to be enabled, or else it
fails to probe:

 [    3.398247] aspeed_gfx 1e6e6000.display: failed to initialize
reserved mem: -22
 [    3.410471] aspeed_gfx: probe of 1e6e6000.display failed with error -22

It was suggested by Michael that 'imply' might be appropriate here.
However, in my testing 'imply' does not set CMA=y . If I start from
both CMA and ASPEED_GFX disabled, and then use menuconfig to enable
ASPEED_GFX, I expected CMA to be enabled. It does not.

In v2[1] of this patch I make the driver depend on ARCH_ASPEED, which
being ARM means it has CMA available. The other case is when enabling
COMPILE_TEST, where the test for HAVE_DMA_CONTIGUOUS will stop the
build break.

CMA only depends on MMU, so we should be safe to select it. I'm open
to other suggstions though.

Cheers,

Joel

[1] https://lore.kernel.org/linux-arm-kernel/20190405081117.27339-1-joel@jms.id.au/