[PATCH v4 3/4] media: raspberrypi: Add support for RP1-CFE

[Tomi Valkeinen]

On 09/09/2024 12:13, Jacopo Mondi wrote:
> Hi Tomi
> 
> On Mon, Sep 09, 2024 at 08:22:59AM GMT, Tomi Valkeinen wrote:
>> Hi Laurent, Jacopo,
>>
>> On 09/09/2024 08:08, Tomi Valkeinen wrote:
>>> Hi,
>>>
>>> On 05/09/2024 14:11, Laurent Pinchart wrote:
>>>> On Thu, Sep 05, 2024 at 06:50:48PM +0800, kernel test robot wrote:
>>>>> Hi Tomi,
>>>>>
>>>>> kernel test robot noticed the following build warnings:
>>>>>
>>>>> [auto build test WARNING on 431c1646e1f86b949fa3685efc50b660a364c2b6]
>>>>>
>>>>> url:    https://github.com/intel-lab-lkp/linux/commits/Tomi-
>>>>> Valkeinen/media-uapi-Add-meta-formats-for-PiSP-FE-config-and-
>>>>> stats/20240904-192729
>>>>> base:   431c1646e1f86b949fa3685efc50b660a364c2b6
>>>>> patch link:    https://lore.kernel.org/r/20240904-rp1-cfe-v4-3-
>>>>> f1b5b3d69c81%40ideasonboard.com
>>>>> patch subject: [PATCH v4 3/4] media: raspberrypi: Add support
>>>>> for RP1-CFE
>>>>> config: m68k-allmodconfig (https://download.01.org/0day-ci/
>>>>> archive/20240905/202409051822.ZzUGw3XQ-lkp at intel.com/config)
>>>>> compiler: m68k-linux-gcc (GCC) 14.1.0
>>>>> reproduce (this is a W=1 build):
>>>>> (https://download.01.org/0day-ci/
>>>>> archive/20240905/202409051822.ZzUGw3XQ-lkp at intel.com/reproduce)
>>>>>
>>>>> If you fix the issue in a separate patch/commit (i.e. not just a
>>>>> new version of
>>>>> the same patch/commit), kindly add following tags
>>>>> | Reported-by: kernel test robot <lkp at intel.com>
>>>>> | Closes: https://lore.kernel.org/oe-kbuild-
>>>>> all/202409051822.ZzUGw3XQ-lkp at intel.com/
>>>>>
>>>>> All warnings (new ones prefixed by >>):
>>>>>
>>>>>>> drivers/media/platform/raspberrypi/rp1-cfe/cfe.c:2445:12:
>>>>>>> warning: 'cfe_runtime_resume' defined but not used
>>>>>>> [-Wunused-function]
>>>>>       2445 | static int cfe_runtime_resume(struct device *dev)
>>>>>            |            ^~~~~~~~~~~~~~~~~~
>>>>>>> drivers/media/platform/raspberrypi/rp1-cfe/cfe.c:2435:12:
>>>>>>> warning: 'cfe_runtime_suspend' defined but not used
>>>>>>> [-Wunused-function]
>>>>>       2435 | static int cfe_runtime_suspend(struct device *dev)
>>>>>            |            ^~~~~~~~~~~~~~~~~~~
>>>>> vim +/cfe_runtime_resume +2445
>>>>> drivers/media/platform/raspberrypi/ rp1-cfe/cfe.c
>>>>
>>>> The recommended way to fix this is to switch from SET_RUNTIME_PM_OPS()
>>>> to RUNTIME_PM_OPS() and use pm_ptr() to set .driver.pm. This being said,
>>>> the driver won't work on a kernel with !CONFIG_PM given how you
>>>> implemented probe() and remove().
>>>>
>>>> The pisp-be driver suffered from the same issue and Jacopo fixed it in
>>>> the last version. You can have a look at implement something similar.
>>>
>>> I can't right away think of any reason to not just depend on CONFIG_PM
>>> and be done with it without any tricks. Do you know if there's a reason?
> 
> We had the same discussion, and even if I would be fine depending on
> CONFIG_PM, supporting !CONFIG_PM is not that much work, I kept it as
> an optional dependency (it was suggested during the review as well)
> 
>>
>> Also, I don't think pisp-be is correct. It just calls
>> pispbe_runtime_resume() in probe() to wake the IP up (which only enables
>> pisp clock), without telling the runtime PM about it. This means the parent
>> device and the suppliers may not be powered up.
> 
> Are you referring to the code currently in the tree or to this patch ?
> https://patchwork.linuxtv.org/project/linux-media/patch/20240904095836.344833-5-jacopo.mondi@ideasonboard.com/

Ah, I missed that one.

I don't think it fixes the issue I mentioned. If we have PM enabled, and 
the parent device requires powering up for the child device (BE) to be 
accessible, the driver will crash when calling pispbe_hw_init(). I think 
you should call pm_runtime_set_active() before calling 
pispbe_runtime_resume().

However, you said above that "supporting !CONFIG_PM is not that much 
work". Maybe not. But how much work is it to get it right (for both PM 
and !PM), and make sure it stays right? =).

Just my opinion, but if there are zero use cases for the !PM, I would 
just go with "depends on PM" to keep the driver simpler, less bug-prone, 
and easier to maintain.

  Tomi