On Wed, 15 Oct 2025 22:41:59 +0200 Loïc Molinari <[email protected]> wrote:
> On 15/10/2025 20:17, Boris Brezillon wrote: > > On Wed, 15 Oct 2025 17:30:12 +0200 > > Loïc Molinari <[email protected]> wrote: > > > >> Don't declare "super_pages" on builds with CONFIG_TRANSPARENT_HUGEPAGE > >> disabled to prevent build error: > >> > >> ERROR: modpost: "super_pages" [drivers/gpu/drm/v3d/v3d.ko] undefined! > > > > I believe this is a bug introduced by the previous commit: the > > compiler probably drops any code between the > > IS_ENABLED(CONFIG_TRANSPARENT_HUGEPAGE) check and the err label > > because IS_ENABLED() evaluates to false at compile time. So I'd squash > > those changes in the previous commit. > > Right, it's been introduced in previous commit. > > > > >> > >> Signed-off-by: Loïc Molinari <[email protected]> > >> --- > >> drivers/gpu/drm/v3d/v3d_drv.h | 2 ++ > >> drivers/gpu/drm/v3d/v3d_gem.c | 2 ++ > >> 2 files changed, 4 insertions(+) > >> > >> diff --git a/drivers/gpu/drm/v3d/v3d_drv.h b/drivers/gpu/drm/v3d/v3d_drv.h > >> index 99a39329bb85..481502104391 100644 > >> --- a/drivers/gpu/drm/v3d/v3d_drv.h > >> +++ b/drivers/gpu/drm/v3d/v3d_drv.h > >> @@ -564,7 +564,9 @@ extern const struct dma_fence_ops v3d_fence_ops; > >> struct dma_fence *v3d_fence_create(struct v3d_dev *v3d, enum v3d_queue > >> q); > >> > >> /* v3d_gem.c */ > >> +#ifdef CONFIG_TRANSPARENT_HUGEPAGE > >> extern bool super_pages; > >> +#endif > >> int v3d_gem_init(struct drm_device *dev); > >> void v3d_gem_destroy(struct drm_device *dev); > >> void v3d_reset_sms(struct v3d_dev *v3d); > >> diff --git a/drivers/gpu/drm/v3d/v3d_gem.c b/drivers/gpu/drm/v3d/v3d_gem.c > >> index 635ff0fabe7e..0039063eb8b2 100644 > >> --- a/drivers/gpu/drm/v3d/v3d_gem.c > >> +++ b/drivers/gpu/drm/v3d/v3d_gem.c > >> @@ -269,7 +269,9 @@ v3d_huge_mnt_init(struct v3d_dev *v3d) > >> * match our usecase. > >> */ > >> > >> +#ifdef CONFIG_TRANSPARENT_HUGEPAGE > >> if (super_pages) > >> +#endif > >> err = drm_gem_huge_mnt_create(&v3d->drm, "within_size"); > > > > Why not > > > > #ifdef CONFIG_TRANSPARENT_HUGEPAGE > > if (super_pages) > > err = drm_gem_huge_mnt_create(&v3d->drm, "within_size"); > > #endif > > > > I guess > > > > if (IS_ENABLED(CONFIG_TRANSPARENT_HUGEPAGE) && super_pages) > > err = drm_gem_huge_mnt_create(&v3d->drm, "within_size"); > > > > would also do, since it's likely to rely on the same optimization the > > previous v3d_gemfs_init() implementation was relying on, but it's > > fragile (not sure what happens when compiled with -O0). > > I'll remove the #ifdef/#endif around the super_pages declaration in > v3d_drv.h because it isn't necessary if super_pages is compiled out in > v3d_huge_mnt_init(). > > In v3d_huge_mnt_init(), I'd add the #ifdef before the ret variable > declaration and the #endif right after the last else so that it's clear > drm_notice("THP is recommended...") is called unconditionally when > CONFIG_TRANSPARENT_HUGEPAGE=n, whatever the optim level. What do you think? First off, I'm not a huge fan of the following pattern #if foo if (xxxx) #endif do_something which also applies to #if foo if (xxxx) do_xxx else if (yyy) do_yyy else #endif do_something I'd rather have do_something duplicated in an #else section like that: #if foo if (xxxx) do_xxx else if (yyy) do_yyy else do_something #else do_something #endif But I'm not even seeing what the problem is here. If you do: int err = 0; #ifdef CONFIG_TRANSPARENT_HUGEPAGE if (super_pages) err = drm_gem_huge_mnt_create(&v3d->drm, "within_size"); #endif if (v3d->drm.huge_mnt) drm_info(&v3d->drm, "Using Transparent Hugepages\n"); else if (err) drm_warn(&v3d->drm, "Can't use Transparent Hugepages (%d)\n", err); else drm_notice(&v3d->drm, "Transparent Hugepage support is recommended for optimal performance on this platform!\n"); You're guaranteed that err=0 and v3d->drm.huge_mnt=NULL when CONFIG_TRANSPARENT_HUGEPAGE=n, so the "THP recommended" message should be displayed unconditionally. Am I missing something?
