[PATCH] drm/panthor: Fix the CONFIG_PM=n case

[Robin Murphy]

On 18/03/2024 1:49 pm, Steven Price wrote:
> On 18/03/2024 13:08, Boris Brezillon wrote:
>> On Mon, 18 Mar 2024 11:31:05 +0000
>> Steven Price <steven.price at arm.com> wrote:
>>
>>> On 18/03/2024 08:58, Boris Brezillon wrote:
>>>> Putting a hard dependency on CONFIG_PM is not possible because of a
>>>> circular dependency issue, and it's actually not desirable either. In
>>>> order to support this use case, we forcibly resume at init time, and
>>>> suspend at unplug time.
>>>>
>>>> Reported-by: kernel test robot <lkp at intel.com>
>>>> Closes: https://lore.kernel.org/oe-kbuild-all/202403031944.EOimQ8WK-lkp@intel.com/
>>>> Signed-off-by: Boris Brezillon <boris.brezillon at collabora.com>
>>>
>>> Reviewed-by: Steven Price <steven.price at arm.com>
>>>
>>>> ---
>>>> Tested by faking CONFIG_PM=n in the driver (basically commenting
>>>> all pm_runtime calls, and making the panthor_device_suspend/resume()
>>>> calls unconditional in the panthor_device_unplug/init() path) since
>>>> CONFIG_ARCH_ROCKCHIP selects CONFIG_PM. Seems to work fine, but I
>>>> can't be 100% sure this will work correctly on a platform that has
>>>> CONFIG_PM=n.
>>>
>>> The same - I can't test this properly :(
>>>
>>> Note that the other option (which AFAICT doesn't cause any problems) is
>>> to "select PM" rather than depend on it - AIUI the 'select' dependency
>>> is considered in the opposite direction by kconfig so won't cause the
>>> dependency loop.
>>
>> Doesn't seem to work with COMPILE_TEST though? I mean, we need
>> something like
>>
>> 	depends on ARM || ARM64 || (COMPILE_TEST && PM)
>> 	...
>> 	select PM
>>
>> but kconfig doesn't like that
> 
> Why do we need the "&& PM" part? Just:
> 
> 	depends on ARM || ARM64 || COMPILE_TEST
> 	...
> 	select PM
> 
> Or at least that appears to work for me.
> 
>> drivers/gpu/drm/panthor/Kconfig:3:error: recursive dependency detected!
>> drivers/gpu/drm/panthor/Kconfig:3:	symbol DRM_PANTHOR depends on
>> PM kernel/power/Kconfig:183:	symbol PM is selected by DRM_PANTHOR
>>
>> which id why I initially when for a depends on PM
>>
>>
>>> Of course if there is actually anyone who has a
>>> platform which can be built !CONFIG_PM then that won't help. But the
>>> inability of anyone to actually properly test this configuration does
>>> worry me a little.
>>
>> Well, as long as it doesn't regress the PM behavior, I think I'm happy
>> to take the risk. Worst case scenario, someone complains that this is
>> not working properly when they do the !PM bringup :-).
> 
> Indeed, I've no objection to this patch - although I really should have
> compiled tested it as Robin pointed out ;)
> 
> But one other thing I've noticed when compile testing it - we don't
> appear to have fully fixed the virt_to_pfn() problem. On x86 with
> COMPILE_TEST I still get an error. Looking at the code it appears that
> virt_to_pfn() isn't available on x86... it overrides asm/page.h and
> doesn't provide a definition. The definition on x86 is hiding in
> asm/xen/page.h.
> 
> Outside of arch code it's only drivers/xen that currently uses that
> function. So I guess it's probably best to do a
> PFN_DOWN(virt_to_phys(...)) instead. Or look to fix x86 :)

FWIW from a quick look it might be cleaner to store the struct page 
pointer for the dummy page - especially since the VA only seems to be 
used once in panthor_device_init() anyway - then use page_to_pfn() at 
the business end.

Cheers,
Robin.