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

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

[kernel test robot]

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@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@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@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202409051822.ZzUGw3XQ-lkp@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

  2434	
> 2435	static int cfe_runtime_suspend(struct device *dev)
  2436	{
  2437		struct platform_device *pdev = to_platform_device(dev);
  2438		struct cfe_device *cfe = platform_get_drvdata(pdev);
  2439	
  2440		clk_disable_unprepare(cfe->clk);
  2441	
  2442		return 0;
  2443	}
  2444	
> 2445	static int cfe_runtime_resume(struct device *dev)
  2446	{
  2447		struct platform_device *pdev = to_platform_device(dev);
  2448		struct cfe_device *cfe = platform_get_drvdata(pdev);
  2449		int ret;
  2450	
  2451		ret = clk_prepare_enable(cfe->clk);
  2452		if (ret) {
  2453			dev_err(dev, "Unable to enable clock\n");
  2454			return ret;
  2455		}
  2456	
  2457		return 0;
  2458	}
  2459	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

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

[Laurent Pinchart]

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@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@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@intel.com>
> | Closes: https://lore.kernel.org/oe-kbuild-all/202409051822.ZzUGw3XQ-lkp@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.

>   2434	
> > 2435	static int cfe_runtime_suspend(struct device *dev)
>   2436	{
>   2437		struct platform_device *pdev = to_platform_device(dev);
>   2438		struct cfe_device *cfe = platform_get_drvdata(pdev);
>   2439	
>   2440		clk_disable_unprepare(cfe->clk);
>   2441	
>   2442		return 0;
>   2443	}
>   2444	
> > 2445	static int cfe_runtime_resume(struct device *dev)
>   2446	{
>   2447		struct platform_device *pdev = to_platform_device(dev);
>   2448		struct cfe_device *cfe = platform_get_drvdata(pdev);
>   2449		int ret;
>   2450	
>   2451		ret = clk_prepare_enable(cfe->clk);
>   2452		if (ret) {
>   2453			dev_err(dev, "Unable to enable clock\n");
>   2454			return ret;
>   2455		}
>   2456	
>   2457		return 0;
>   2458	}
>   2459	

-- 
Regards,

Laurent Pinchart

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

[Tomi Valkeinen]

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@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@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@intel.com>
>> | Closes: https://lore.kernel.org/oe-kbuild-all/202409051822.ZzUGw3XQ-lkp@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?

  Tomi

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

[Tomi Valkeinen]

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@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@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@intel.com>
>>> | Closes: https://lore.kernel.org/oe-kbuild- 
>>> all/202409051822.ZzUGw3XQ-lkp@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?

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.

  Tomi

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

[Jacopo Mondi]

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@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@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@intel.com>
> > > > | Closes: https://lore.kernel.org/oe-kbuild-
> > > > all/202409051822.ZzUGw3XQ-lkp@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/

>
>  Tomi
>
>

Re: [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@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@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@intel.com>
>>>>> | Closes: https://lore.kernel.org/oe-kbuild-
>>>>> all/202409051822.ZzUGw3XQ-lkp@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

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

[Jacopo Mondi]

Hi Tomi

On Mon, Sep 09, 2024 at 01:04:35PM GMT, Tomi Valkeinen wrote:
> 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@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@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@intel.com>
> > > > > > | Closes: https://lore.kernel.org/oe-kbuild-
> > > > > > all/202409051822.ZzUGw3XQ-lkp@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().

As discussed, this is not a problem currently for BE, but indeed you
have a point.

Sad note: most of all the occurrences of "grep set_active" in
drivers/media/platform/ show that set_active() is used as I've done in
my patch

>
> 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.
>

I don't see a use case for !PM and we confirmed with RPi they don't
need to support it. During the review of a previous version of the BE
driver iirc I've been asked to support !PM but I'm not sure I recall
the reasons.

>  Tomi
>

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

[Laurent Pinchart]

On Mon, Sep 09, 2024 at 03:29:30PM +0200, Jacopo Mondi wrote:
> On Mon, Sep 09, 2024 at 01:04:35PM GMT, Tomi Valkeinen wrote:
> > On 09/09/2024 12:13, Jacopo Mondi wrote:
> > > On Mon, Sep 09, 2024 at 08:22:59AM GMT, Tomi Valkeinen wrote:
> > > > On 09/09/2024 08:08, Tomi Valkeinen wrote:
> > > > > 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@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@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@intel.com>
> > > > > > > | Closes: https://lore.kernel.org/oe-kbuild-
> > > > > > > all/202409051822.ZzUGw3XQ-lkp@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().
> 
> As discussed, this is not a problem currently for BE, but indeed you
> have a point.
> 
> Sad note: most of all the occurrences of "grep set_active" in
> drivers/media/platform/ show that set_active() is used as I've done in
> my patch
> 
> > 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.

I'm fine with that, and for platform drivers, that's my preferred
option. Sakari ?

> I don't see a use case for !PM and we confirmed with RPi they don't
> need to support it. During the review of a previous version of the BE
> driver iirc I've been asked to support !PM but I'm not sure I recall
> the reasons.

I hope it wasn't me :-)

-- 
Regards,

Laurent Pinchart

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

[Sakari Ailus]

Hi Laurent,

On Mon, Sep 09, 2024 at 04:45:16PM +0300, Laurent Pinchart wrote:
> On Mon, Sep 09, 2024 at 03:29:30PM +0200, Jacopo Mondi wrote:
> > On Mon, Sep 09, 2024 at 01:04:35PM GMT, Tomi Valkeinen wrote:
> > > On 09/09/2024 12:13, Jacopo Mondi wrote:
> > > > On Mon, Sep 09, 2024 at 08:22:59AM GMT, Tomi Valkeinen wrote:
> > > > > On 09/09/2024 08:08, Tomi Valkeinen wrote:
> > > > > > 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@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@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@intel.com>
> > > > > > > > | Closes: https://lore.kernel.org/oe-kbuild-
> > > > > > > > all/202409051822.ZzUGw3XQ-lkp@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().
> > 
> > As discussed, this is not a problem currently for BE, but indeed you
> > have a point.
> > 
> > Sad note: most of all the occurrences of "grep set_active" in
> > drivers/media/platform/ show that set_active() is used as I've done in
> > my patch
> > 
> > > 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.
> 
> I'm fine with that, and for platform drivers, that's my preferred
> option. Sakari ?

I'm concerned with your (?) recent finding that many architectures don't
have support for CONFIG_PM. In this case the device is very unlikely to be
used outside ARM(64) so I guess it's fine.

> 
> > I don't see a use case for !PM and we confirmed with RPi they don't
> > need to support it. During the review of a previous version of the BE
> > driver iirc I've been asked to support !PM but I'm not sure I recall
> > the reasons.
> 
> I hope it wasn't me :-)

Me neither. Although it'd be nice: CONFIG_PM isn't a hardware specific
option as such. As one part of the kernel requires !CONFIG_PM and another
CONFIG_PM, we can expect problems, at least in principle.

Ideally all architectures would support it so CONFIG_PM could be removed
and we could say the problem has been solved. :-)

-- 
Regards,

Sakari Ailus

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

[Jacopo Mondi]

Hi Sakari, Tomi

On Mon, Sep 09, 2024 at 03:52:42PM GMT, Sakari Ailus wrote:
> Hi Laurent,
>
> On Mon, Sep 09, 2024 at 04:45:16PM +0300, Laurent Pinchart wrote:
> > On Mon, Sep 09, 2024 at 03:29:30PM +0200, Jacopo Mondi wrote:
> > > On Mon, Sep 09, 2024 at 01:04:35PM GMT, Tomi Valkeinen wrote:
> > > > On 09/09/2024 12:13, Jacopo Mondi wrote:
> > > > > On Mon, Sep 09, 2024 at 08:22:59AM GMT, Tomi Valkeinen wrote:
> > > > > > On 09/09/2024 08:08, Tomi Valkeinen wrote:
> > > > > > > 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@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@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@intel.com>
> > > > > > > > > | Closes: https://lore.kernel.org/oe-kbuild-
> > > > > > > > > all/202409051822.ZzUGw3XQ-lkp@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().
> > >
> > > As discussed, this is not a problem currently for BE, but indeed you
> > > have a point.

I admit the runtime_pm intrinsics are obscure to me, but Laurent just
made me notice something.

Consider the following scenario

*) Kernel compiled with CONFIG_PM
*) i2c sensor driver that supports both CONFIG_PM and !CONFIG_PM by:
  *) Manually power up the sensor during probe
  *) Call pm_runtime_enable() and pm_runtime_set_active() at the end
     of the probe routine after having accessed the chip over i2c
     (like most, if not all the i2c drivers in mainline do including
     ccs)

All these drivers work, and during the probe routine before accessing
the HW, they don't need to power up the parent i2c controller.

Might it be that during probe() the parent is guaranteed to be enabled ?

I add a look in the driver-core and pm Documentation/ but found
nothing.

A quick stroll in driver/base/ got me to __device_attach() and it
seems parents are powered up before attaching a driver to a device
(which in my understanding should be what ends up calling probe()).
Clearly I've no real understanding of what I'm talking about when it
comes to driver-core, so take this with a grain of salt.

> > >
> > > Sad note: most of all the occurrences of "grep set_active" in
> > > drivers/media/platform/ show that set_active() is used as I've done in
> > > my patch
> > >
> > > > 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.
> >
> > I'm fine with that, and for platform drivers, that's my preferred
> > option. Sakari ?
>
> I'm concerned with your (?) recent finding that many architectures don't
> have support for CONFIG_PM. In this case the device is very unlikely to be
> used outside ARM(64) so I guess it's fine.
>

Also, this IP is RPi specific, and the !CONFIG_PM case is not used or
tested on Pi.

However, I think this current patch is correct (assuming the above
reasoning on i2c sensor drivers is correct) and doesn't require
CONFIG_PM, so I would be tempted to keep this version.

> >
> > > I don't see a use case for !PM and we confirmed with RPi they don't
> > > need to support it. During the review of a previous version of the BE
> > > driver iirc I've been asked to support !PM but I'm not sure I recall
> > > the reasons.
> >
> > I hope it wasn't me :-)
>
> Me neither. Although it'd be nice: CONFIG_PM isn't a hardware specific
> option as such. As one part of the kernel requires !CONFIG_PM and another
> CONFIG_PM, we can expect problems, at least in principle.
>
> Ideally all architectures would support it so CONFIG_PM could be removed
> and we could say the problem has been solved. :-)
>
> --
> Regards,
>
> Sakari Ailus

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

[Sakari Ailus]

Hi Jacopo,

On Tue, Sep 10, 2024 at 11:19:57AM +0200, Jacopo Mondi wrote:
> Hi Sakari, Tomi
> 
> On Mon, Sep 09, 2024 at 03:52:42PM GMT, Sakari Ailus wrote:
> > Hi Laurent,
> >
> > On Mon, Sep 09, 2024 at 04:45:16PM +0300, Laurent Pinchart wrote:
> > > On Mon, Sep 09, 2024 at 03:29:30PM +0200, Jacopo Mondi wrote:
> > > > On Mon, Sep 09, 2024 at 01:04:35PM GMT, Tomi Valkeinen wrote:
> > > > > On 09/09/2024 12:13, Jacopo Mondi wrote:
> > > > > > On Mon, Sep 09, 2024 at 08:22:59AM GMT, Tomi Valkeinen wrote:
> > > > > > > On 09/09/2024 08:08, Tomi Valkeinen wrote:
> > > > > > > > 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@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@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@intel.com>
> > > > > > > > > > | Closes: https://lore.kernel.org/oe-kbuild-
> > > > > > > > > > all/202409051822.ZzUGw3XQ-lkp@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().
> > > >
> > > > As discussed, this is not a problem currently for BE, but indeed you
> > > > have a point.
> 
> I admit the runtime_pm intrinsics are obscure to me, but Laurent just
> made me notice something.
> 
> Consider the following scenario
> 
> *) Kernel compiled with CONFIG_PM
> *) i2c sensor driver that supports both CONFIG_PM and !CONFIG_PM by:
>   *) Manually power up the sensor during probe
>   *) Call pm_runtime_enable() and pm_runtime_set_active() at the end
>      of the probe routine after having accessed the chip over i2c
>      (like most, if not all the i2c drivers in mainline do including
>      ccs)
> 
> All these drivers work, and during the probe routine before accessing
> the HW, they don't need to power up the parent i2c controller.

This isn't done explicitly by the I²C drivers, indeed but...

> 
> Might it be that during probe() the parent is guaranteed to be enabled ?

...yes.

> 
> I add a look in the driver-core and pm Documentation/ but found
> nothing.
> 
> A quick stroll in driver/base/ got me to __device_attach() and it
> seems parents are powered up before attaching a driver to a device
> (which in my understanding should be what ends up calling probe()).
> Clearly I've no real understanding of what I'm talking about when it
> comes to driver-core, so take this with a grain of salt.

This only works with runtime PM enabled.

> 
> > > >
> > > > Sad note: most of all the occurrences of "grep set_active" in
> > > > drivers/media/platform/ show that set_active() is used as I've done in
> > > > my patch
> > > >
> > > > > 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.
> > >
> > > I'm fine with that, and for platform drivers, that's my preferred
> > > option. Sakari ?
> >
> > I'm concerned with your (?) recent finding that many architectures don't
> > have support for CONFIG_PM. In this case the device is very unlikely to be
> > used outside ARM(64) so I guess it's fine.
> >
> 
> Also, this IP is RPi specific, and the !CONFIG_PM case is not used or
> tested on Pi.
> 
> However, I think this current patch is correct (assuming the above
> reasoning on i2c sensor drivers is correct) and doesn't require
> CONFIG_PM, so I would be tempted to keep this version.

I understand the current patch does depend on CONFIG_PM: it requires
runtime PM to be operational to start the clock, for instance.

> 
> > >
> > > > I don't see a use case for !PM and we confirmed with RPi they don't
> > > > need to support it. During the review of a previous version of the BE
> > > > driver iirc I've been asked to support !PM but I'm not sure I recall
> > > > the reasons.
> > >
> > > I hope it wasn't me :-)
> >
> > Me neither. Although it'd be nice: CONFIG_PM isn't a hardware specific
> > option as such. As one part of the kernel requires !CONFIG_PM and another
> > CONFIG_PM, we can expect problems, at least in principle.
> >
> > Ideally all architectures would support it so CONFIG_PM could be removed
> > and we could say the problem has been solved. :-)

-- 
Kidnregards,

Sakari Ailus

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

[Jacopo Mondi]

Hi Sakari

On Tue, Sep 10, 2024 at 09:25:56AM GMT, Sakari Ailus wrote:
> Hi Jacopo,
>
> On Tue, Sep 10, 2024 at 11:19:57AM +0200, Jacopo Mondi wrote:
> > Hi Sakari, Tomi
> >
> > On Mon, Sep 09, 2024 at 03:52:42PM GMT, Sakari Ailus wrote:
> > > Hi Laurent,
> > >
> > > On Mon, Sep 09, 2024 at 04:45:16PM +0300, Laurent Pinchart wrote:
> > > > On Mon, Sep 09, 2024 at 03:29:30PM +0200, Jacopo Mondi wrote:
> > > > > On Mon, Sep 09, 2024 at 01:04:35PM GMT, Tomi Valkeinen wrote:
> > > > > > On 09/09/2024 12:13, Jacopo Mondi wrote:
> > > > > > > On Mon, Sep 09, 2024 at 08:22:59AM GMT, Tomi Valkeinen wrote:
> > > > > > > > On 09/09/2024 08:08, Tomi Valkeinen wrote:
> > > > > > > > > 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@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@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@intel.com>
> > > > > > > > > > > | Closes: https://lore.kernel.org/oe-kbuild-
> > > > > > > > > > > all/202409051822.ZzUGw3XQ-lkp@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().
> > > > >
> > > > > As discussed, this is not a problem currently for BE, but indeed you
> > > > > have a point.
> >
> > I admit the runtime_pm intrinsics are obscure to me, but Laurent just
> > made me notice something.
> >
> > Consider the following scenario
> >
> > *) Kernel compiled with CONFIG_PM
> > *) i2c sensor driver that supports both CONFIG_PM and !CONFIG_PM by:
> >   *) Manually power up the sensor during probe
> >   *) Call pm_runtime_enable() and pm_runtime_set_active() at the end
> >      of the probe routine after having accessed the chip over i2c
> >      (like most, if not all the i2c drivers in mainline do including
> >      ccs)
> >
> > All these drivers work, and during the probe routine before accessing
> > the HW, they don't need to power up the parent i2c controller.
>
> This isn't done explicitly by the I²C drivers, indeed but...
>
> >
> > Might it be that during probe() the parent is guaranteed to be enabled ?
>
> ...yes.
>

Unrelated, but I am wrong the driver core calls pm_request_idle() on
the just probed device ? Does this mean drivers doesn't have to do
that by themseleves ? (it's not a big deal, as request_idle() doesn't
change the usage counter)

> >
> > I add a look in the driver-core and pm Documentation/ but found
> > nothing.
> >
> > A quick stroll in driver/base/ got me to __device_attach() and it
> > seems parents are powered up before attaching a driver to a device
> > (which in my understanding should be what ends up calling probe()).
> > Clearly I've no real understanding of what I'm talking about when it
> > comes to driver-core, so take this with a grain of salt.
>
> This only works with runtime PM enabled.
>

It's the CONFIG_PM case that was worrying for Tomi

> >
> > > > >
> > > > > Sad note: most of all the occurrences of "grep set_active" in
> > > > > drivers/media/platform/ show that set_active() is used as I've done in
> > > > > my patch
> > > > >
> > > > > > 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.
> > > >
> > > > I'm fine with that, and for platform drivers, that's my preferred
> > > > option. Sakari ?
> > >
> > > I'm concerned with your (?) recent finding that many architectures don't
> > > have support for CONFIG_PM. In this case the device is very unlikely to be
> > > used outside ARM(64) so I guess it's fine.
> > >
> >
> > Also, this IP is RPi specific, and the !CONFIG_PM case is not used or
> > tested on Pi.
> >
> > However, I think this current patch is correct (assuming the above
> > reasoning on i2c sensor drivers is correct) and doesn't require
> > CONFIG_PM, so I would be tempted to keep this version.
>
> I understand the current patch does depend on CONFIG_PM: it requires
> runtime PM to be operational to start the clock, for instance.
>

Where do you see that ?

This version still calls pispbe_runtime_resume() to power up the IP,
it doesn't go through runtime_pm:
https://patchwork.linuxtv.org/project/linux-media/patch/20240904095836.344833-5-jacopo.mondi@ideasonboard.com/

Thanks!

> >
> > > >
> > > > > I don't see a use case for !PM and we confirmed with RPi they don't
> > > > > need to support it. During the review of a previous version of the BE
> > > > > driver iirc I've been asked to support !PM but I'm not sure I recall
> > > > > the reasons.
> > > >
> > > > I hope it wasn't me :-)
> > >
> > > Me neither. Although it'd be nice: CONFIG_PM isn't a hardware specific
> > > option as such. As one part of the kernel requires !CONFIG_PM and another
> > > CONFIG_PM, we can expect problems, at least in principle.
> > >
> > > Ideally all architectures would support it so CONFIG_PM could be removed
> > > and we could say the problem has been solved. :-)
>
> --
> Kidnregards,
>
> Sakari Ailus

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

[Sakari Ailus]

Hi Jacopo,

On Tue, Sep 10, 2024 at 11:42:06AM +0200, Jacopo Mondi wrote:
> > > > > > > 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().
> > > > > >
> > > > > > As discussed, this is not a problem currently for BE, but indeed you
> > > > > > have a point.
> > >
> > > I admit the runtime_pm intrinsics are obscure to me, but Laurent just
> > > made me notice something.
> > >
> > > Consider the following scenario
> > >
> > > *) Kernel compiled with CONFIG_PM
> > > *) i2c sensor driver that supports both CONFIG_PM and !CONFIG_PM by:
> > >   *) Manually power up the sensor during probe
> > >   *) Call pm_runtime_enable() and pm_runtime_set_active() at the end
> > >      of the probe routine after having accessed the chip over i2c
> > >      (like most, if not all the i2c drivers in mainline do including
> > >      ccs)
> > >
> > > All these drivers work, and during the probe routine before accessing
> > > the HW, they don't need to power up the parent i2c controller.
> >
> > This isn't done explicitly by the I²C drivers, indeed but...
> >
> > >
> > > Might it be that during probe() the parent is guaranteed to be enabled ?
> >
> > ...yes.
> >
> 
> Unrelated, but I am wrong the driver core calls pm_request_idle() on
> the just probed device ? Does this mean drivers doesn't have to do
> that by themseleves ? (it's not a big deal, as request_idle() doesn't
> change the usage counter)

Seems like that. This could be omitted from drivers then.

> 
> > >
> > > I add a look in the driver-core and pm Documentation/ but found
> > > nothing.
> > >
> > > A quick stroll in driver/base/ got me to __device_attach() and it
> > > seems parents are powered up before attaching a driver to a device
> > > (which in my understanding should be what ends up calling probe()).
> > > Clearly I've no real understanding of what I'm talking about when it
> > > comes to driver-core, so take this with a grain of salt.
> >
> > This only works with runtime PM enabled.
> >
> 
> It's the CONFIG_PM case that was worrying for Tomi
> 
> > >
> > > > > >
> > > > > > Sad note: most of all the occurrences of "grep set_active" in
> > > > > > drivers/media/platform/ show that set_active() is used as I've done in
> > > > > > my patch
> > > > > >
> > > > > > > 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.
> > > > >
> > > > > I'm fine with that, and for platform drivers, that's my preferred
> > > > > option. Sakari ?
> > > >
> > > > I'm concerned with your (?) recent finding that many architectures don't
> > > > have support for CONFIG_PM. In this case the device is very unlikely to be
> > > > used outside ARM(64) so I guess it's fine.
> > > >
> > >
> > > Also, this IP is RPi specific, and the !CONFIG_PM case is not used or
> > > tested on Pi.
> > >
> > > However, I think this current patch is correct (assuming the above
> > > reasoning on i2c sensor drivers is correct) and doesn't require
> > > CONFIG_PM, so I would be tempted to keep this version.
> >
> > I understand the current patch does depend on CONFIG_PM: it requires
> > runtime PM to be operational to start the clock, for instance.
> >
> 
> Where do you see that ?
> 
> This version still calls pispbe_runtime_resume() to power up the IP,
> it doesn't go through runtime_pm:
> https://patchwork.linuxtv.org/project/linux-media/patch/20240904095836.344833-5-jacopo.mondi@ideasonboard.com/

cfe_runtime_resume() is only called via runtime PM.

> 
> Thanks!
> 
> > >
> > > > >
> > > > > > I don't see a use case for !PM and we confirmed with RPi they don't
> > > > > > need to support it. During the review of a previous version of the BE
> > > > > > driver iirc I've been asked to support !PM but I'm not sure I recall
> > > > > > the reasons.
> > > > >
> > > > > I hope it wasn't me :-)
> > > >
> > > > Me neither. Although it'd be nice: CONFIG_PM isn't a hardware specific
> > > > option as such. As one part of the kernel requires !CONFIG_PM and another
> > > > CONFIG_PM, we can expect problems, at least in principle.
> > > >
> > > > Ideally all architectures would support it so CONFIG_PM could be removed
> > > > and we could say the problem has been solved. :-)

-- 
Kind regards,

Sakari Ailus

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

[Tomi Valkeinen]

On 10/09/2024 12:19, Jacopo Mondi wrote:

> However, I think this current patch is correct (assuming the above
> reasoning on i2c sensor drivers is correct) and doesn't require
> CONFIG_PM, so I would be tempted to keep this version.

I think the existence of this discussion alone proves my point that we 
should only support PM-case, unless !PM is a requirement =).

But if you do want to keep !PM:

Is there a reason why not mark the device as active with 
pm_runtime_set_active() after calling pispbe_runtime_resume and before 
accessing the device? That feels like the most logical way to use the 
function, and it would be right regardless whether the core will enable 
the parents before probe() or not.

And not related to the BE or CFE drivers, but it strikes me odd that to 
support PM and !PM we need to play with these tricks. I think the core 
should just do the right thing if the driver does pm_runtime_get_sync() 
even with !PM (although maybe the time has passed to be able to do that).

  Tomi

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

[Laurent Pinchart]

On Tue, Sep 10, 2024 at 09:55:20AM +0000, Sakari Ailus wrote:
> On Tue, Sep 10, 2024 at 11:42:06AM +0200, Jacopo Mondi wrote:
> > > > > > > > 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().
> > > > > > >
> > > > > > > As discussed, this is not a problem currently for BE, but indeed you
> > > > > > > have a point.
> > > >
> > > > I admit the runtime_pm intrinsics are obscure to me, but Laurent just
> > > > made me notice something.
> > > >
> > > > Consider the following scenario
> > > >
> > > > *) Kernel compiled with CONFIG_PM
> > > > *) i2c sensor driver that supports both CONFIG_PM and !CONFIG_PM by:
> > > >   *) Manually power up the sensor during probe
> > > >   *) Call pm_runtime_enable() and pm_runtime_set_active() at the end
> > > >      of the probe routine after having accessed the chip over i2c
> > > >      (like most, if not all the i2c drivers in mainline do including
> > > >      ccs)
> > > >
> > > > All these drivers work, and during the probe routine before accessing
> > > > the HW, they don't need to power up the parent i2c controller.
> > >
> > > This isn't done explicitly by the I²C drivers, indeed but...
> > >
> > > >
> > > > Might it be that during probe() the parent is guaranteed to be enabled ?
> > >
> > > ...yes.
> > >
> > 
> > Unrelated, but I am wrong the driver core calls pm_request_idle() on
> > the just probed device ? Does this mean drivers doesn't have to do
> > that by themseleves ? (it's not a big deal, as request_idle() doesn't
> > change the usage counter)
> 
> Seems like that. This could be omitted from drivers then.

Is that guaranteed, or is it an implementation detail that could change
at any time ?

> > > > I add a look in the driver-core and pm Documentation/ but found
> > > > nothing.
> > > >
> > > > A quick stroll in driver/base/ got me to __device_attach() and it
> > > > seems parents are powered up before attaching a driver to a device
> > > > (which in my understanding should be what ends up calling probe()).
> > > > Clearly I've no real understanding of what I'm talking about when it
> > > > comes to driver-core, so take this with a grain of salt.
> > >
> > > This only works with runtime PM enabled.
> > 
> > It's the CONFIG_PM case that was worrying for Tomi
> > 
> > > > > > > Sad note: most of all the occurrences of "grep set_active" in
> > > > > > > drivers/media/platform/ show that set_active() is used as I've done in
> > > > > > > my patch
> > > > > > >
> > > > > > > > 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.
> > > > > >
> > > > > > I'm fine with that, and for platform drivers, that's my preferred
> > > > > > option. Sakari ?
> > > > >
> > > > > I'm concerned with your (?) recent finding that many architectures don't
> > > > > have support for CONFIG_PM. In this case the device is very unlikely to be
> > > > > used outside ARM(64) so I guess it's fine.
> > > > >
> > > >
> > > > Also, this IP is RPi specific, and the !CONFIG_PM case is not used or
> > > > tested on Pi.
> > > >
> > > > However, I think this current patch is correct (assuming the above
> > > > reasoning on i2c sensor drivers is correct) and doesn't require
> > > > CONFIG_PM, so I would be tempted to keep this version.
> > >
> > > I understand the current patch does depend on CONFIG_PM: it requires
> > > runtime PM to be operational to start the clock, for instance.
> > 
> > Where do you see that ?
> > 
> > This version still calls pispbe_runtime_resume() to power up the IP,
> > it doesn't go through runtime_pm:
> > https://patchwork.linuxtv.org/project/linux-media/patch/20240904095836.344833-5-jacopo.mondi@ideasonboard.com/
> 
> cfe_runtime_resume() is only called via runtime PM.
> 
> > Thanks!
> > 
> > > > > > > I don't see a use case for !PM and we confirmed with RPi they don't
> > > > > > > need to support it. During the review of a previous version of the BE
> > > > > > > driver iirc I've been asked to support !PM but I'm not sure I recall
> > > > > > > the reasons.
> > > > > >
> > > > > > I hope it wasn't me :-)
> > > > >
> > > > > Me neither. Although it'd be nice: CONFIG_PM isn't a hardware specific
> > > > > option as such. As one part of the kernel requires !CONFIG_PM and another
> > > > > CONFIG_PM, we can expect problems, at least in principle.
> > > > >
> > > > > Ideally all architectures would support it so CONFIG_PM could be removed
> > > > > and we could say the problem has been solved. :-)

-- 
Regards,

Laurent Pinchart

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

[Laurent Pinchart]

On Tue, Sep 10, 2024 at 12:56:38PM +0300, Tomi Valkeinen wrote:
> On 10/09/2024 12:19, Jacopo Mondi wrote:
> 
> > However, I think this current patch is correct (assuming the above
> > reasoning on i2c sensor drivers is correct) and doesn't require
> > CONFIG_PM, so I would be tempted to keep this version.
> 
> I think the existence of this discussion alone proves my point that we 
> should only support PM-case, unless !PM is a requirement =).

For me it proves there's a dire need to document the runtime PM API in a
way that a human could understand :-)

> But if you do want to keep !PM:
> 
> Is there a reason why not mark the device as active with 
> pm_runtime_set_active() after calling pispbe_runtime_resume and before 
> accessing the device? That feels like the most logical way to use the 
> function, and it would be right regardless whether the core will enable 
> the parents before probe() or not.

Does pm_runtime_set_active() resume the parent ?

> And not related to the BE or CFE drivers, but it strikes me odd that to 
> support PM and !PM we need to play with these tricks. I think the core 
> should just do the right thing if the driver does pm_runtime_get_sync() 
> even with !PM (although maybe the time has passed to be able to do that).

The runtime PM concepts are nice, but the API is wrong in my opinion.
Instead of being designed to expose the internals of runtime PM, it
should focus on usability from a driver point of view first.

-- 
Regards,

Laurent Pinchart

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

[Tomi Valkeinen]

On 10/09/2024 13:06, Laurent Pinchart wrote:

>>> Unrelated, but I am wrong the driver core calls pm_request_idle() on
>>> the just probed device ? Does this mean drivers doesn't have to do
>>> that by themseleves ? (it's not a big deal, as request_idle() doesn't
>>> change the usage counter)
>>
>> Seems like that. This could be omitted from drivers then.
> 
> Is that guaranteed, or is it an implementation detail that could change
> at any time ?

The runtime_pm.rst says:

> It may be desirable to suspend the device once ->probe() has finished.
> Therefore the driver core uses the asynchronous pm_request_idle() to submit a
> request to execute the subsystem-level idle callback for the device at that
> time.  A driver that makes use of the runtime autosuspend feature may want to
> update the last busy mark before returning from ->probe().

But I can't find any mention of the dependencies (parent, suppliers) 
being powered up automatically for probe.

  Tomi

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

[Jacopo Mondi]

Hi Tomi

On Tue, Sep 10, 2024 at 12:56:38PM GMT, Tomi Valkeinen wrote:
> On 10/09/2024 12:19, Jacopo Mondi wrote:
>
> > However, I think this current patch is correct (assuming the above
> > reasoning on i2c sensor drivers is correct) and doesn't require
> > CONFIG_PM, so I would be tempted to keep this version.
>
> I think the existence of this discussion alone proves my point that we
> should only support PM-case, unless !PM is a requirement =).
>
> But if you do want to keep !PM:
>
> Is there a reason why not mark the device as active with
> pm_runtime_set_active() after calling pispbe_runtime_resume and before

cargo-cult ?

> accessing the device? That feels like the most logical way to use the
> function, and it would be right regardless whether the core will enable the
> parents before probe() or not.

Possibly more accurate, but there's no guarantee it's correct. The
peripheral might have requirements on the clock or power rails
enablement order and some might be managed by the parent. I know we're
talking hypothesis but my point is that there's not correctness
guarantee we can enforce unless the parent is powered up when the
device probes ?

Anyway, I'll defer the call to the group: either keep the patch as it
is right now on the list, or go full runtime_pm. I understand there is
no reason to care about !CONFIG_PM but somehow I feel "bad" in listing
it as a dependency if the peripheral can actually work without it.
Maybe I should just ignore that feeling ?


>
> And not related to the BE or CFE drivers, but it strikes me odd that to
> support PM and !PM we need to play with these tricks. I think the core
> should just do the right thing if the driver does pm_runtime_get_sync() even
> with !PM (although maybe the time has passed to be able to do that).

I wish that was the case.

>
>  Tomi
>

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

[Sakari Ailus]

Hi Laurent,

On Tue, Sep 10, 2024 at 01:11:37PM +0300, Laurent Pinchart wrote:
> On Tue, Sep 10, 2024 at 12:56:38PM +0300, Tomi Valkeinen wrote:
> > On 10/09/2024 12:19, Jacopo Mondi wrote:
> > 
> > > However, I think this current patch is correct (assuming the above
> > > reasoning on i2c sensor drivers is correct) and doesn't require
> > > CONFIG_PM, so I would be tempted to keep this version.
> > 
> > I think the existence of this discussion alone proves my point that we 
> > should only support PM-case, unless !PM is a requirement =).
> 
> For me it proves there's a dire need to document the runtime PM API in a
> way that a human could understand :-)
> 
> > But if you do want to keep !PM:
> > 
> > Is there a reason why not mark the device as active with 
> > pm_runtime_set_active() after calling pispbe_runtime_resume and before 
> > accessing the device? That feels like the most logical way to use the 
> > function, and it would be right regardless whether the core will enable 
> > the parents before probe() or not.
> 
> Does pm_runtime_set_active() resume the parent ?

No, it simply sets the device's runtime PM status.

> 
> > And not related to the BE or CFE drivers, but it strikes me odd that to 
> > support PM and !PM we need to play with these tricks. I think the core 
> > should just do the right thing if the driver does pm_runtime_get_sync() 
> > even with !PM (although maybe the time has passed to be able to do that).
> 
> The runtime PM concepts are nice, but the API is wrong in my opinion.
> Instead of being designed to expose the internals of runtime PM, it
> should focus on usability from a driver point of view first.

It's been moving a little bit to that direction, largely with new helper
functions.

I²C devices have been powered on for probe since commit a76e9bd89ae70 .
Relation to runtime PM wasn't considered at the time, apparently.

-- 
Kind regards,

Sakari Ailus

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

[Tomi Valkeinen]

Hi,

On 10/09/2024 13:11, Laurent Pinchart wrote:
> On Tue, Sep 10, 2024 at 12:56:38PM +0300, Tomi Valkeinen wrote:
>> On 10/09/2024 12:19, Jacopo Mondi wrote:
>>
>>> However, I think this current patch is correct (assuming the above
>>> reasoning on i2c sensor drivers is correct) and doesn't require
>>> CONFIG_PM, so I would be tempted to keep this version.
>>
>> I think the existence of this discussion alone proves my point that we
>> should only support PM-case, unless !PM is a requirement =).
> 
> For me it proves there's a dire need to document the runtime PM API in a
> way that a human could understand :-)

That too, but it's a parallel track =).

>> But if you do want to keep !PM:
>>
>> Is there a reason why not mark the device as active with
>> pm_runtime_set_active() after calling pispbe_runtime_resume and before
>> accessing the device? That feels like the most logical way to use the
>> function, and it would be right regardless whether the core will enable
>> the parents before probe() or not.
> 
> Does pm_runtime_set_active() resume the parent ?

I thought so, but I'm not sure anymore:

> if the device has a parent and the parent is not active, and the
> parent's power.ignore_children flag is unset, the device's status
 > cannot be set to RPM_ACTIVE, so -EBUSY is returned in that case.

It does resume the suppliers, though.

So using pm_runtime_set_active() only works if you know that the parent 
has been activated earlier? If there's such a guarantee for probe() and 
remove(), does it then mean that you can only call 
pm_runtime_set_active() in probe()/remove()...

  Tomi

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

[Tomi Valkeinen]

On 10/09/2024 13:18, Jacopo Mondi wrote:
> Hi Tomi
> 
> On Tue, Sep 10, 2024 at 12:56:38PM GMT, Tomi Valkeinen wrote:
>> On 10/09/2024 12:19, Jacopo Mondi wrote:
>>
>>> However, I think this current patch is correct (assuming the above
>>> reasoning on i2c sensor drivers is correct) and doesn't require
>>> CONFIG_PM, so I would be tempted to keep this version.
>>
>> I think the existence of this discussion alone proves my point that we
>> should only support PM-case, unless !PM is a requirement =).
>>
>> But if you do want to keep !PM:
>>
>> Is there a reason why not mark the device as active with
>> pm_runtime_set_active() after calling pispbe_runtime_resume and before
> 
> cargo-cult ?
> 
>> accessing the device? That feels like the most logical way to use the
>> function, and it would be right regardless whether the core will enable the
>> parents before probe() or not.
> 
> Possibly more accurate, but there's no guarantee it's correct. The
> peripheral might have requirements on the clock or power rails
> enablement order and some might be managed by the parent. I know we're
> talking hypothesis but my point is that there's not correctness
> guarantee we can enforce unless the parent is powered up when the
> device probes ?
> 
> Anyway, I'll defer the call to the group: either keep the patch as it
> is right now on the list, or go full runtime_pm. I understand there is
> no reason to care about !CONFIG_PM but somehow I feel "bad" in listing
> it as a dependency if the peripheral can actually work without it.
> Maybe I should just ignore that feeling ?

The runtime PM is just a software construct, so all peripherals can work 
without it.

I decided to ignore the feeling, very much based on this thread, and 
sent CFE v5 with it depending on PM.

  Tomi