All of lore.kernel.org
 help / color / mirror / Atom feed
From: Robin Murphy <robin.murphy@arm.com>
To: Steven Price <steven.price@arm.com>,
	Boris Brezillon <boris.brezillon@collabora.com>
Cc: Liviu Dudau <liviu.dudau@arm.com>,
	dri-devel@lists.freedesktop.org, kernel@collabora.com,
	kernel test robot <lkp@intel.com>
Subject: Re: [PATCH] drm/panthor: Fix the CONFIG_PM=n case
Date: Mon, 18 Mar 2024 14:04:19 +0000	[thread overview]
Message-ID: <689eed7c-836f-4ca8-9d57-f7ff045a6cf8@arm.com> (raw)
In-Reply-To: <5c9257cb-8307-4f9e-9323-2ed367c48a11@arm.com>

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@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@intel.com>
>>>> Closes: https://lore.kernel.org/oe-kbuild-all/202403031944.EOimQ8WK-lkp@intel.com/
>>>> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
>>>
>>> Reviewed-by: Steven Price <steven.price@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.

  reply	other threads:[~2024-03-18 14:04 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-03-18  8:58 [PATCH] drm/panthor: Fix the CONFIG_PM=n case Boris Brezillon
2024-03-18 11:17 ` Liviu Dudau
2024-03-18 11:31 ` Steven Price
2024-03-18 13:08   ` Boris Brezillon
2024-03-18 13:49     ` Steven Price
2024-03-18 14:04       ` Robin Murphy [this message]
2024-03-18 14:18       ` Boris Brezillon
2024-03-18 14:34         ` Steven Price
2024-03-18 14:49           ` Boris Brezillon
2024-03-18 14:58             ` Steven Price
2024-03-18 12:18 ` Robin Murphy
2024-03-18 12:57   ` Boris Brezillon

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=689eed7c-836f-4ca8-9d57-f7ff045a6cf8@arm.com \
    --to=robin.murphy@arm.com \
    --cc=boris.brezillon@collabora.com \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=kernel@collabora.com \
    --cc=liviu.dudau@arm.com \
    --cc=lkp@intel.com \
    --cc=steven.price@arm.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.