Re: [PATCH v4 04/13] drm/panfrost: Move shrinker initialization and unplug one level down

[Steven Price]

On 12/08/2026 02:19, Adrián Larumbe wrote:
> Hi Steven,
> 
> On 30.07.2026 11:39, Steven Price wrote:
>> On 29/07/2026 03:54, Adrián Larumbe wrote:
>>> Since the moment we call drm_dev_register() the device should be in a
>>> position to accept jobs, so it's best if the shrinker is already
>>> initialized by then.
>>>
>>> On top of that, make shrinker functions take an panfrost_device pointer
>>> like other functions in the same sequence and rename them accordingly.
>>>
>>> Essentially mimic the init/fini behaviour in Panthor.
>>>
>>> Signed-off-by: Adrián Larumbe <adrian.larumbe@xxxxxxxxxxxxx>
>>> ---
>>>  drivers/gpu/drm/panfrost/panfrost_device.c       |  8 +++++++-
>>>  drivers/gpu/drm/panfrost/panfrost_drv.c          |  6 ------
>>>  drivers/gpu/drm/panfrost/panfrost_gem.c          | 19 ++++++++++---------
>>>  drivers/gpu/drm/panfrost/panfrost_gem.h          |  6 +++---
>>>  drivers/gpu/drm/panfrost/panfrost_gem_shrinker.c |  8 ++------
>>>  5 files changed, 22 insertions(+), 25 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/panfrost/panfrost_device.c b/drivers/gpu/drm/panfrost/panfrost_device.c
>>> index d8421fd6a662..03905495aee2 100644
>>> --- a/drivers/gpu/drm/panfrost/panfrost_device.c
>>> +++ b/drivers/gpu/drm/panfrost/panfrost_device.c
>>> @@ -281,9 +281,14 @@ int panfrost_device_init(struct panfrost_device *pfdev)
>>>  	if (err)
>>>  		goto out_job;
>>>  
>>> -	panfrost_gem_init(pfdev);
>>> +	err = panfrost_gem_init(pfdev);
>>> +	if (err)
>>> +		goto out_perfcnt;
>>>  
>>>  	return 0;
>>> +
>>> +out_perfcnt:
>>> +	panfrost_perfcnt_fini(pfdev);
>>>  out_job:
>>>  	panfrost_jm_fini(pfdev);
>>>  out_mmu:
>>> @@ -305,6 +310,7 @@ int panfrost_device_init(struct panfrost_device *pfdev)
>>>  
>>>  void panfrost_device_fini(struct panfrost_device *pfdev)
>>>  {
>>> +	panfrost_gem_shrinker_fini(pfdev);
>>>  	panfrost_perfcnt_fini(pfdev);
>>>  	panfrost_jm_fini(pfdev);
>>>  	panfrost_mmu_fini(pfdev);
>>> diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
>>> index 2e931fa90da0..331a3bd5b98c 100644
>>> --- a/drivers/gpu/drm/panfrost/panfrost_drv.c
>>> +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
>>> @@ -1002,14 +1002,9 @@ static int panfrost_probe(struct platform_device *pdev)
>>>  	if (err < 0)
>>>  		goto err_out1;
>>>  
>>> -	err = panfrost_gem_shrinker_init(&pfdev->base);
>>> -	if (err)
>>> -		goto err_out2;
>>>  
>>>  	return 0;
>>>  
>>> -err_out2:
>>> -	drm_dev_unregister(&pfdev->base);
>>>  err_out1:
>>>  	pm_runtime_disable(pfdev->base.dev);
>>>  	panfrost_device_fini(pfdev);
>>> @@ -1023,7 +1018,6 @@ static void panfrost_remove(struct platform_device *pdev)
>>>  	struct panfrost_device *pfdev = platform_get_drvdata(pdev);
>>>  
>>>  	drm_dev_unregister(&pfdev->base);
>>> -	panfrost_gem_shrinker_cleanup(&pfdev->base);
>>>  
>>>  	pm_runtime_get_sync(pfdev->base.dev);
>>>  	pm_runtime_disable(pfdev->base.dev);
>>> diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.c b/drivers/gpu/drm/panfrost/panfrost_gem.c
>>> index 3a7fce428898..aa3a604b3439 100644
>>> --- a/drivers/gpu/drm/panfrost/panfrost_gem.c
>>> +++ b/drivers/gpu/drm/panfrost/panfrost_gem.c
>>> @@ -15,20 +15,21 @@
>>>  #include "panfrost_gem.h"
>>>  #include "panfrost_mmu.h"
>>>  
>>> -void panfrost_gem_init(struct panfrost_device *pfdev)
>>> +int panfrost_gem_init(struct panfrost_device *pfdev)
>>>  {
>>>  	int err;
>>>  
>>>  	if (IS_ENABLED(CONFIG_TRANSPARENT_HUGEPAGE) &&
>>> -	    !panfrost_transparent_hugepage)
>>> -		return;
>>> +	    panfrost_transparent_hugepage) {
>>
>> Note you haven't quite inverted the condition here, but I don't think
>> the new condition is wrong - we skip the otherwise no-op helpers. The
>> old condition seems a bit backward. Indeed I'm not really sure why we
>> bother with the IS_ENABLED() check at all. But the new code looks fine.
> 
> I've heeded your advice and done away with the IS_ENABLED() check in v5.
> I'm curious about why that would work though. Is it because the linker initialises
> symbols marked as 'extern' to 0 when they aren't resolved?
> 
> Otherwise I would've assumed the compiler would throw out a missing definition
> error at link time, but then again I'm unsure about this.

Hmm, I hadn't actually spotted that panfrost_transparent_hugepage wasn't
defined unless CONFIG_TRANSPARENT_HUGEPAGE. Usually this is handled with
a bit of macro magic that #defines the symbol if it's not relevant. I
would expect the linker to complain if this symbol still exists in the
final output.

One possibility is that the compiler is spotting that the
drm_gem_huge_mnt_create() and drm_gem_get_huge_mnt() functions are all
no-ops in this case, so the entire if() branch can be removed (along
with the condition check). Although I'm not sure how safe it is to rely
on such compiler optimisations.

Thanks,
Steve

>> Reviewed-by: Steven Price <steven.price@xxxxxxx>
>>
>> Thanks,
>> Steve
>>
>>> +		err = drm_gem_huge_mnt_create(&pfdev->base, "within_size");
>>> +		if (drm_gem_get_huge_mnt(&pfdev->base))
>>> +			drm_info(&pfdev->base, "Using Transparent Hugepage\n");
>>> +		else if (err)
>>> +			drm_warn(&pfdev->base,
>>> +				 "Can't use Transparent Hugepage (%d)\n", err);
>>> +	}
>>>  
>>> -	err = drm_gem_huge_mnt_create(&pfdev->base, "within_size");
>>> -	if (drm_gem_get_huge_mnt(&pfdev->base))
>>> -		drm_info(&pfdev->base, "Using Transparent Hugepage\n");
>>> -	else if (err)
>>> -		drm_warn(&pfdev->base, "Can't use Transparent Hugepage (%d)\n",
>>> -			 err);
>>> +	return panfrost_gem_shrinker_init(pfdev);
>>>  }
>>>  
>>>  #ifdef CONFIG_DEBUG_FS
>>> diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.h b/drivers/gpu/drm/panfrost/panfrost_gem.h
>>> index 79d4377019e9..d6e55c2e4d86 100644
>>> --- a/drivers/gpu/drm/panfrost/panfrost_gem.h
>>> +++ b/drivers/gpu/drm/panfrost/panfrost_gem.h
>>> @@ -129,7 +129,7 @@ drm_mm_node_to_panfrost_mapping(struct drm_mm_node *node)
>>>  	return container_of(node, struct panfrost_gem_mapping, mmnode);
>>>  }
>>>  
>>> -void panfrost_gem_init(struct panfrost_device *pfdev);
>>> +int panfrost_gem_init(struct panfrost_device *pfdev);
>>>  
>>>  struct drm_gem_object *panfrost_gem_create_object(struct drm_device *dev, size_t size);
>>>  
>>> @@ -154,8 +154,8 @@ panfrost_gem_mapping_get(struct panfrost_gem_object *bo,
>>>  void panfrost_gem_mapping_put(struct panfrost_gem_mapping *mapping);
>>>  void panfrost_gem_teardown_mappings_locked(struct panfrost_gem_object *bo);
>>>  
>>> -int panfrost_gem_shrinker_init(struct drm_device *dev);
>>> -void panfrost_gem_shrinker_cleanup(struct drm_device *dev);
>>> +int panfrost_gem_shrinker_init(struct panfrost_device *pfdev);
>>> +void panfrost_gem_shrinker_fini(struct panfrost_device *pfdev);
>>>  
>>>  void panfrost_gem_set_label(struct drm_gem_object *obj, const char *label);
>>>  int panfrost_gem_sync(struct drm_gem_object *obj, u32 type,
>>> diff --git a/drivers/gpu/drm/panfrost/panfrost_gem_shrinker.c b/drivers/gpu/drm/panfrost/panfrost_gem_shrinker.c
>>> index 2fe967a90bcb..fefae87535d6 100644
>>> --- a/drivers/gpu/drm/panfrost/panfrost_gem_shrinker.c
>>> +++ b/drivers/gpu/drm/panfrost/panfrost_gem_shrinker.c
>>> @@ -95,10 +95,8 @@ panfrost_gem_shrinker_scan(struct shrinker *shrinker, struct shrink_control *sc)
>>>   *
>>>   * This function registers and sets up the panfrost shrinker.
>>>   */
>>> -int panfrost_gem_shrinker_init(struct drm_device *dev)
>>> +int panfrost_gem_shrinker_init(struct panfrost_device *pfdev)
>>>  {
>>> -	struct panfrost_device *pfdev = to_panfrost_device(dev);
>>> -
>>>  	pfdev->shrinker = shrinker_alloc(0, "drm-panfrost");
>>>  	if (!pfdev->shrinker)
>>>  		return -ENOMEM;
>>> @@ -118,10 +116,8 @@ int panfrost_gem_shrinker_init(struct drm_device *dev)
>>>   *
>>>   * This function unregisters the panfrost shrinker.
>>>   */
>>> -void panfrost_gem_shrinker_cleanup(struct drm_device *dev)
>>> +void panfrost_gem_shrinker_fini(struct panfrost_device *pfdev)
>>>  {
>>> -	struct panfrost_device *pfdev = to_panfrost_device(dev);
>>> -
>>>  	if (pfdev->shrinker)
>>>  		shrinker_free(pfdev->shrinker);
>>>  }
>>>
> 
> Adrian Larumbe