1. Patchew
  2. linux
  3. [PATCH v4 00/13] drm: Reduce page tables overhead with THP
  4. View patch

[PATCH v4 08/13] drm/v3d: Fix builds with CONFIG_TRANSPARENT_HUGEPAGE=n

Loïc Molinari posted 13 patches 2 months ago
There is a newer version of this series
[PATCH v4 08/13] drm/v3d: Fix builds with CONFIG_TRANSPARENT_HUGEPAGE=n
Posted by Loïc Molinari 2 months ago 
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!

Signed-off-by: Loïc Molinari <loic.molinari@collabora.com>
---
 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");
 
 	if (v3d->drm.huge_mnt)
-- 
2.47.3
Re: [PATCH v4 08/13] drm/v3d: Fix builds with CONFIG_TRANSPARENT_HUGEPAGE=n
Posted by Boris Brezillon 2 months ago 
On Wed, 15 Oct 2025 17:30:12 +0200
Loïc Molinari <loic.molinari@collabora.com> 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.

> 
> Signed-off-by: Loïc Molinari <loic.molinari@collabora.com>
> ---
>  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).

>  
>  	if (v3d->drm.huge_mnt)
Re: [PATCH v4 08/13] drm/v3d: Fix builds with CONFIG_TRANSPARENT_HUGEPAGE=n
Posted by Loïc Molinari 2 months ago 
On 15/10/2025 20:17, Boris Brezillon wrote:
> On Wed, 15 Oct 2025 17:30:12 +0200
> Loïc Molinari <loic.molinari@collabora.com> 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 <loic.molinari@collabora.com>
>> ---
>>   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?

> 
>>   
>>   	if (v3d->drm.huge_mnt)
>
Re: [PATCH v4 08/13] drm/v3d: Fix builds with CONFIG_TRANSPARENT_HUGEPAGE=n
Posted by Boris Brezillon 2 months ago 
On Wed, 15 Oct 2025 22:41:59 +0200
Loïc Molinari <loic.molinari@collabora.com> wrote:

> On 15/10/2025 20:17, Boris Brezillon wrote:
> > On Wed, 15 Oct 2025 17:30:12 +0200
> > Loïc Molinari <loic.molinari@collabora.com> 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 <loic.molinari@collabora.com>
> >> ---
> >>   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?
Re: [PATCH v4 08/13] drm/v3d: Fix builds with CONFIG_TRANSPARENT_HUGEPAGE=n
Posted by Loïc Molinari 2 months ago 
Hi Boris,

On 16/10/2025 07:56, Boris Brezillon wrote:
> On Wed, 15 Oct 2025 22:41:59 +0200
> Loïc Molinari <loic.molinari@collabora.com> wrote:
> 
>> On 15/10/2025 20:17, Boris Brezillon wrote:
>>> On Wed, 15 Oct 2025 17:30:12 +0200
>>> Loïc Molinari <loic.molinari@collabora.com> 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 <loic.molinari@collabora.com>
>>>> ---
>>>>    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?

It doesn't really matter here but I just thought it would be cleaner to 
explicitly let just the drm_notice() because the compiler doesn't know 
v3d->drm.huge_mnt is always NULL here and would emit a branch in 
CONFIG_TRANSPARENT_HUGEPAGE=n builds. I know your dislike for this 
pattern now, so I will stick to the suggestion :)

Loïc
Re: [PATCH v4 08/13] drm/v3d: Fix builds with CONFIG_TRANSPARENT_HUGEPAGE=n
Posted by Boris Brezillon 2 months ago 
On Thu, 16 Oct 2025 09:09:26 +0200
Loïc Molinari <loic.molinari@collabora.com> wrote:

> Hi Boris,
> 
> On 16/10/2025 07:56, Boris Brezillon wrote:
> > On Wed, 15 Oct 2025 22:41:59 +0200
> > Loïc Molinari <loic.molinari@collabora.com> wrote:
> >   
> >> On 15/10/2025 20:17, Boris Brezillon wrote:  
> >>> On Wed, 15 Oct 2025 17:30:12 +0200
> >>> Loïc Molinari <loic.molinari@collabora.com> 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 <loic.molinari@collabora.com>
> >>>> ---
> >>>>    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?  
> 
> It doesn't really matter here but I just thought it would be cleaner to 
> explicitly let just the drm_notice() because the compiler doesn't know 
> v3d->drm.huge_mnt is always NULL here and would emit a branch in 
> CONFIG_TRANSPARENT_HUGEPAGE=n builds.

Okay, well. I think it would matter if this was a hot-path, maybe? But
that's clearly not the case, this code is called once at probe time.

> I know your dislike for this 
> pattern now, so I will stick to the suggestion :)

Actually, I would probably stick to the original

	if (IS_ENABLED(CONFIG_TRANSPARENT_HUGEPAGE) && super_pages)
		err = drm_gem_huge_mnt_create(&v3d->drm, "within_size");

pattern since it worked so far. If -O0 is really a problem (didn't
check if it is), and v3d maintainers care about it (I doubt anyone
forces -O0 to be honest), they'll fix that in a follow-up patchset.

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.