Re: [PATCH v4] s5p-fimc: Add runtime PM support in the mem-to-mem driver

2011-09-06 Thread Sylwester Nawrocki 
On 09/05/2011 11:20 PM, Sakari Ailus wrote:
 +static int fimc_suspend(struct device *dev)
 +{
 +  struct fimc_dev *fimc = dev_get_drvdata(dev);
 +
 +  dbg(fimc%d: state: 0x%lx, fimc-id, fimc-state);
 +
 +  if (test_and_set_bit(ST_LPM,fimc-state))
 +  return 0;
 +  if (fimc_capture_busy(fimc))
 +  return fimc_capture_suspend(fimc);

 Now that fimc_capture_suspend()  returns -EBUSY always, is this intended
 behavious or do you plan to change this later on?

 No, it's by no means the intended behaviour. This patch is only a part of the
 whole picture, but I thought it's independent from the MC related patches
 which are on hold and could be merged independently. Moreover the FIMC driver
 is broken without this patch on Exynos4, if the boot loader doesn't enable
 the related power domain permanently. So I thought it should be merged
 regardless of the fate of the capture PM support patch which depends on the
 MC related patches.
 
 Right, I agree the patch has enough merits for merging.
 
 Here is the capture PM patch for your critics;) http://tinyurl.com/4yj8z4t
 
 I'll take a look at it once you post it on the list. ;)

It's been on the lists for some time already, this is the fourth version:
https://patchwork.kernel.org/patch/1119562/
However it doesn't include a small fix I have added after posting v4, 
which is available in the above git repository.


 Not that it'd be really easy to do this properly; the sensors, for example,
 probably need a clock from the ISP and I2C before they can continue. The
 OMAP 3 ISP driver does attempt to do this but doesn't handle these
 dependencies.

 I'm not handling the device PM dependencies explicitly in this driver either.

 But it's assured the I2C bus device is registered first, then the camera host
 device, and finally the I2C client devices.
 AFAIU the PM core should call PM suspend helpers on the subdev/host drivers
 in order: I2C clients, the camera host and I2C bus. And for the resume 
 helpers
 the sequence should be reversed.
 
 In my understanding it is not ensured that the I2C bus driver starts before
 the media device parent does (as it is controlling the sensors' power
 state). The same goes for suspend. Or am I missing something?

AFAIU PM core maintains private list of its active devices which it then walks
when preparing, suspending, resuming and 'completing' subsystems.
Please check pm_device_add() and dpm_start_suspend() for instance. It looks like
the list can only be reordered through returning -EAGAIN from subsystem prepare
helper or by calling implicitly pm_device_move().
I might be missing some important details though, it would be best to clarify
these things on linux-pm ML.

 
 The sensor drivers do not implement their standard PM helper callbacks,
 their are just controlled directly through s_power op by the host driver.


 I'm not suggesting this should be part of the patch, just thought of asking
 it. :)

 First of all I'm not entirely happy with this code. The are some issues in
 the v4l2-mem2mem framework which I plan to address when time permits. I think
 it wasn't designed in PM use cases in mind. Plus PM support in Exynos4 
 platform
 (including drivers) is rather not yet stable in the mainline kernel. So I was
 having hard time to make this PM code working in the mem-to-mem device.
 But it's now done and only a per frame clock gating is still missing.
 This is a quite complex topic, to get everything right, in line with all
 frameworks involved.


 +  return fimc_m2m_suspend(fimc);

 Does pending mean there are further images to process in a queue, or just
 that driver is busy one?

 It means the driver got an ownership of a pair of buffers and is about to or
 is already processing them. In any case fimc_m2m_suspend() will wait for
 only those two buffers to be processed, without dequeuing them back to user
 space. They will be returned back to user space when the driver's resume 
 helper
 is called.
 
 I think this is a good approach. Processing the buffers takes a fraction of
 a second. If one would cancel this it would unnecessarily complicate the
 user space.

Yes, and applications should not really care much about device power state 
transitions.

 

 +#endif /* CONFIG_PM_SLEEP */
 +
 ...
 diff --git a/drivers/media/video/s5p-fimc/fimc-reg.c 
 b/drivers/media/video/s5p-fimc/fimc-reg.c
 index 4893b2d..938dadf 100644
 --- a/drivers/media/video/s5p-fimc/fimc-reg.c
 +++ b/drivers/media/video/s5p-fimc/fimc-reg.c
 @@ -30,7 +30,7 @@ void fimc_hw_reset(struct fimc_dev *dev)
cfg = readl(dev-regs + S5P_CIGCTRL);
cfg |= (S5P_CIGCTRL_SWRST | S5P_CIGCTRL_IRQ_LEVEL);
writel(cfg, dev-regs + S5P_CIGCTRL);
 -  udelay(1000);
 +  udelay(10);

 Good catch. Large delays such as this one should have either used msleep()
 or usleep_range(). If a smaller one does, all the better.

 Yeah, now this delay gets in the way every time the device is brought from
 no power to fully operational state, e.g. the

Re: [PATCH v4] s5p-fimc: Add runtime PM support in the mem-to-mem driver

2011-09-05 Thread Sakari Ailus 
Hi Sylwester,

On Tue, Aug 30, 2011 at 05:00:39PM +0200, Sylwester Nawrocki wrote:
 Add runtime PM and system sleep support in the memory-to-memory driver.
 This is required to enable the device operation on Exynos4 SoCs. This patch
 prevents system boot failure when the driver is compiled in, as it now
 tries to access its I/O memory without first enabling the corresponding
 power domain.
 
 The camera capture device suspend/resume is not fully covered,
 the capture device is just powered on/off during the video node
 open/close. However this enables it's normal operation on Exynos4 SoCs.
 
 Signed-off-by: Sylwester Nawrocki s.nawro...@samsung.com
 Signed-off-by: Kyungmin Park kyungmin.p...@samsung.com
 ---
 
 I'm resending this patch after few minor changes since v3:
  - added the driver dependency on PM_RUNTIME
  - corrected the error path in probe() 
  - s/*_in_use/_busy
  - edited the commit message
 
 ---
  drivers/media/video/Kconfig |2 +-
  drivers/media/video/s5p-fimc/fimc-capture.c |   18 ++
  drivers/media/video/s5p-fimc/fimc-core.c|  279 
 ---
  drivers/media/video/s5p-fimc/fimc-core.h|   16 +-
  drivers/media/video/s5p-fimc/fimc-reg.c |2 +-
  5 files changed, 237 insertions(+), 80 deletions(-)
 
 diff --git a/drivers/media/video/Kconfig b/drivers/media/video/Kconfig
 index f574dc0..14326d7 100644
 --- a/drivers/media/video/Kconfig
 +++ b/drivers/media/video/Kconfig
 @@ -950,7 +950,7 @@ config VIDEO_MX2
  
  config  VIDEO_SAMSUNG_S5P_FIMC
   tristate Samsung S5P and EXYNOS4 camera host interface driver
 - depends on VIDEO_DEV  VIDEO_V4L2  PLAT_S5P
 + depends on VIDEO_V4L2  PLAT_S5P  PM_RUNTIME
   select VIDEOBUF2_DMA_CONTIG
   select V4L2_MEM2MEM_DEV
   ---help---
 diff --git a/drivers/media/video/s5p-fimc/fimc-capture.c 
 b/drivers/media/video/s5p-fimc/fimc-capture.c
 index 0d730e5..ea74e4b 100644
 --- a/drivers/media/video/s5p-fimc/fimc-capture.c
 +++ b/drivers/media/video/s5p-fimc/fimc-capture.c
 @@ -17,6 +17,7 @@
  #include linux/interrupt.h
  #include linux/device.h
  #include linux/platform_device.h
 +#include linux/pm_runtime.h
  #include linux/list.h
  #include linux/slab.h
  #include linux/clk.h
 @@ -196,6 +197,16 @@ static int fimc_stop_capture(struct fimc_dev *fimc)
   return 0;
  }
  
 +int fimc_capture_suspend(struct fimc_dev *fimc)
 +{
 + return -EBUSY;
 +}
 +
 +int fimc_capture_resume(struct fimc_dev *fimc)
 +{
 + return 0;
 +}
 +
  static int start_streaming(struct vb2_queue *q)
  {
   struct fimc_ctx *ctx = q-drv_priv;
 @@ -381,9 +392,14 @@ static int fimc_capture_open(struct file *file)
   if (fimc_m2m_active(fimc))
   return -EBUSY;
  
 + ret = pm_runtime_get_sync(fimc-pdev-dev);
 + if (ret)
 + return ret;
 +
   if (++fimc-vid_cap.refcnt == 1) {
   ret = fimc_isp_subdev_init(fimc, 0);
   if (ret) {
 + pm_runtime_put_sync(fimc-pdev-dev);
   fimc-vid_cap.refcnt--;
   return -EIO;
   }
 @@ -411,6 +427,8 @@ static int fimc_capture_close(struct file *file)
   fimc_subdev_unregister(fimc);
   }
  
 + pm_runtime_put_sync(fimc-pdev-dev);
 +
   return 0;
  }
  
 diff --git a/drivers/media/video/s5p-fimc/fimc-core.c 
 b/drivers/media/video/s5p-fimc/fimc-core.c
 index aa55066..7ca8091 100644
 --- a/drivers/media/video/s5p-fimc/fimc-core.c
 +++ b/drivers/media/video/s5p-fimc/fimc-core.c
 @@ -18,6 +18,7 @@
  #include linux/interrupt.h
  #include linux/device.h
  #include linux/platform_device.h
 +#include linux/pm_runtime.h
  #include linux/list.h
  #include linux/io.h
  #include linux/slab.h
 @@ -301,7 +302,6 @@ int fimc_set_scaler_info(struct fimc_ctx *ctx)
  static void fimc_m2m_job_finish(struct fimc_ctx *ctx, int vb_state)
  {
   struct vb2_buffer *src_vb, *dst_vb;
 - struct fimc_dev *fimc = ctx-fimc_dev;
  
   if (!ctx || !ctx-m2m_ctx)
   return;
 @@ -312,39 +312,48 @@ static void fimc_m2m_job_finish(struct fimc_ctx *ctx, 
 int vb_state)
   if (src_vb  dst_vb) {
   v4l2_m2m_buf_done(src_vb, vb_state);
   v4l2_m2m_buf_done(dst_vb, vb_state);
 - v4l2_m2m_job_finish(fimc-m2m.m2m_dev, ctx-m2m_ctx);
 + v4l2_m2m_job_finish(ctx-fimc_dev-m2m.m2m_dev,
 + ctx-m2m_ctx);
   }
  }
  
  /* Complete the transaction which has been scheduled for execution. */
 -static void fimc_m2m_shutdown(struct fimc_ctx *ctx)
 +static int fimc_m2m_shutdown(struct fimc_ctx *ctx)
  {
   struct fimc_dev *fimc = ctx-fimc_dev;
   int ret;
  
   if (!fimc_m2m_pending(fimc))
 - return;
 + return 0;
  
   fimc_ctx_state_lock_set(FIMC_CTX_SHUT, ctx);
  
   ret = wait_event_timeout(fimc-irq_queue,
  !fimc_ctx_state_is_set(FIMC_CTX_SHUT, ctx),

Re: [PATCH v4] s5p-fimc: Add runtime PM support in the mem-to-mem driver

2011-09-05 Thread Sylwester Nawrocki 
Hi Sakari,

thanks for the comments.

On 09/05/2011 08:06 AM, Sakari Ailus wrote:
 Hi Sylwester,
 
 On Tue, Aug 30, 2011 at 05:00:39PM +0200, Sylwester Nawrocki wrote:
 Add runtime PM and system sleep support in the memory-to-memory driver.
 This is required to enable the device operation on Exynos4 SoCs. This patch
 prevents system boot failure when the driver is compiled in, as it now
 tries to access its I/O memory without first enabling the corresponding
 power domain.

 The camera capture device suspend/resume is not fully covered,
 the capture device is just powered on/off during the video node
 open/close. However this enables it's normal operation on Exynos4 SoCs.

 Signed-off-by: Sylwester Nawrockis.nawro...@samsung.com
 Signed-off-by: Kyungmin Parkkyungmin.p...@samsung.com
 ---

 I'm resending this patch after few minor changes since v3:
   - added the driver dependency on PM_RUNTIME
   - corrected the error path in probe()
   - s/*_in_use/_busy
   - edited the commit message

 ---
   drivers/media/video/Kconfig |2 +-
   drivers/media/video/s5p-fimc/fimc-capture.c |   18 ++
   drivers/media/video/s5p-fimc/fimc-core.c|  279 
 ---
   drivers/media/video/s5p-fimc/fimc-core.h|   16 +-
   drivers/media/video/s5p-fimc/fimc-reg.c |2 +-
   5 files changed, 237 insertions(+), 80 deletions(-)

...
 diff --git a/drivers/media/video/s5p-fimc/fimc-capture.c 
 b/drivers/media/video/s5p-fimc/fimc-capture.c
 index 0d730e5..ea74e4b 100644
 --- a/drivers/media/video/s5p-fimc/fimc-capture.c
 +++ b/drivers/media/video/s5p-fimc/fimc-capture.c
 @@ -17,6 +17,7 @@
   #includelinux/interrupt.h
   #includelinux/device.h
   #includelinux/platform_device.h
 +#includelinux/pm_runtime.h
   #includelinux/list.h
   #includelinux/slab.h
   #includelinux/clk.h
 @@ -196,6 +197,16 @@ static int fimc_stop_capture(struct fimc_dev *fimc)
  return 0;
   }

 +int fimc_capture_suspend(struct fimc_dev *fimc)
 +{
 +return -EBUSY;
 +}
 +
 +int fimc_capture_resume(struct fimc_dev *fimc)
 +{
 +return 0;
 +}
 +
   static int start_streaming(struct vb2_queue *q)
   {
  struct fimc_ctx *ctx = q-drv_priv;
 @@ -381,9 +392,14 @@ static int fimc_capture_open(struct file *file)
  if (fimc_m2m_active(fimc))
  return -EBUSY;

 +ret = pm_runtime_get_sync(fimc-pdev-dev);
 +if (ret)
 +return ret;
 +
  if (++fimc-vid_cap.refcnt == 1) {
  ret = fimc_isp_subdev_init(fimc, 0);
  if (ret) {
 +pm_runtime_put_sync(fimc-pdev-dev);
  fimc-vid_cap.refcnt--;
  return -EIO;
  }
 @@ -411,6 +427,8 @@ static int fimc_capture_close(struct file *file)
  fimc_subdev_unregister(fimc);
  }

 +pm_runtime_put_sync(fimc-pdev-dev);
 +
  return 0;
   }

 diff --git a/drivers/media/video/s5p-fimc/fimc-core.c 
 b/drivers/media/video/s5p-fimc/fimc-core.c
 index aa55066..7ca8091 100644
 --- a/drivers/media/video/s5p-fimc/fimc-core.c
 +++ b/drivers/media/video/s5p-fimc/fimc-core.c
 @@ -18,6 +18,7 @@
   #includelinux/interrupt.h
   #includelinux/device.h
   #includelinux/platform_device.h
 +#includelinux/pm_runtime.h
   #includelinux/list.h
   #includelinux/io.h
   #includelinux/slab.h
 @@ -301,7 +302,6 @@ int fimc_set_scaler_info(struct fimc_ctx *ctx)
   static void fimc_m2m_job_finish(struct fimc_ctx *ctx, int vb_state)
   {
  struct vb2_buffer *src_vb, *dst_vb;
 -struct fimc_dev *fimc = ctx-fimc_dev;

  if (!ctx || !ctx-m2m_ctx)
  return;
 @@ -312,39 +312,48 @@ static void fimc_m2m_job_finish(struct fimc_ctx *ctx, 
 int vb_state)
  if (src_vb  dst_vb) {
  v4l2_m2m_buf_done(src_vb, vb_state);
  v4l2_m2m_buf_done(dst_vb, vb_state);
 -v4l2_m2m_job_finish(fimc-m2m.m2m_dev, ctx-m2m_ctx);
 +v4l2_m2m_job_finish(ctx-fimc_dev-m2m.m2m_dev,
 +ctx-m2m_ctx);
  }
   }

   /* Complete the transaction which has been scheduled for execution. */
 -static void fimc_m2m_shutdown(struct fimc_ctx *ctx)
 +static int fimc_m2m_shutdown(struct fimc_ctx *ctx)
   {
  struct fimc_dev *fimc = ctx-fimc_dev;
  int ret;

  if (!fimc_m2m_pending(fimc))
 -return;
 +return 0;

  fimc_ctx_state_lock_set(FIMC_CTX_SHUT, ctx);

  ret = wait_event_timeout(fimc-irq_queue,
 !fimc_ctx_state_is_set(FIMC_CTX_SHUT, ctx),
 FIMC_SHUTDOWN_TIMEOUT);
 -/*
 - * In case of a timeout the buffers are not released in the interrupt
 - * handler so return them here with the error flag set, if there are
 - * any on the queue.
 - */
 -if (ret == 0)
 -fimc_m2m_job_finish(ctx, VB2_BUF_STATE_ERROR);
 +
 +return ret == 0 ? -ETIMEDOUT : ret;
 +}
 +
 +static int start_streaming(struct vb2_queue *q)
 +{
 +struct fimc_ctx *ctx = q-drv_priv;
 +int ret;
 +
 +ret =

Re: [PATCH v4] s5p-fimc: Add runtime PM support in the mem-to-mem driver

2011-09-05 Thread Sakari Ailus 
On Mon, Sep 05, 2011 at 09:01:35PM +0200, Sylwester Nawrocki wrote:
 Hi Sakari,
 
 thanks for the comments.

You're welcome!

 On 09/05/2011 08:06 AM, Sakari Ailus wrote:
  Hi Sylwester,
  
  On Tue, Aug 30, 2011 at 05:00:39PM +0200, Sylwester Nawrocki wrote:
  Add runtime PM and system sleep support in the memory-to-memory driver.
  This is required to enable the device operation on Exynos4 SoCs. This patch
  prevents system boot failure when the driver is compiled in, as it now
  tries to access its I/O memory without first enabling the corresponding
  power domain.
 
  The camera capture device suspend/resume is not fully covered,
  the capture device is just powered on/off during the video node
  open/close. However this enables it's normal operation on Exynos4 SoCs.
 
  Signed-off-by: Sylwester Nawrockis.nawro...@samsung.com
  Signed-off-by: Kyungmin Parkkyungmin.p...@samsung.com
  ---
 
  I'm resending this patch after few minor changes since v3:
- added the driver dependency on PM_RUNTIME
- corrected the error path in probe()
- s/*_in_use/_busy
- edited the commit message
 
  ---
drivers/media/video/Kconfig |2 +-
drivers/media/video/s5p-fimc/fimc-capture.c |   18 ++
drivers/media/video/s5p-fimc/fimc-core.c|  279 
  ---
drivers/media/video/s5p-fimc/fimc-core.h|   16 +-
drivers/media/video/s5p-fimc/fimc-reg.c |2 +-
5 files changed, 237 insertions(+), 80 deletions(-)
 
 ...
  diff --git a/drivers/media/video/s5p-fimc/fimc-capture.c 
  b/drivers/media/video/s5p-fimc/fimc-capture.c
  index 0d730e5..ea74e4b 100644
  --- a/drivers/media/video/s5p-fimc/fimc-capture.c
  +++ b/drivers/media/video/s5p-fimc/fimc-capture.c
  @@ -17,6 +17,7 @@
#includelinux/interrupt.h
#includelinux/device.h
#includelinux/platform_device.h
  +#includelinux/pm_runtime.h
#includelinux/list.h
#includelinux/slab.h
#includelinux/clk.h
  @@ -196,6 +197,16 @@ static int fimc_stop_capture(struct fimc_dev *fimc)
 return 0;
}
 
  +int fimc_capture_suspend(struct fimc_dev *fimc)
  +{
  +  return -EBUSY;
  +}
  +
  +int fimc_capture_resume(struct fimc_dev *fimc)
  +{
  +  return 0;
  +}
  +
static int start_streaming(struct vb2_queue *q)
{
 struct fimc_ctx *ctx = q-drv_priv;
  @@ -381,9 +392,14 @@ static int fimc_capture_open(struct file *file)
 if (fimc_m2m_active(fimc))
 return -EBUSY;
 
  +  ret = pm_runtime_get_sync(fimc-pdev-dev);
  +  if (ret)
  +  return ret;
  +
 if (++fimc-vid_cap.refcnt == 1) {
 ret = fimc_isp_subdev_init(fimc, 0);
 if (ret) {
  +  pm_runtime_put_sync(fimc-pdev-dev);
 fimc-vid_cap.refcnt--;
 return -EIO;
 }
  @@ -411,6 +427,8 @@ static int fimc_capture_close(struct file *file)
 fimc_subdev_unregister(fimc);
 }
 
  +  pm_runtime_put_sync(fimc-pdev-dev);
  +
 return 0;
}
 
  diff --git a/drivers/media/video/s5p-fimc/fimc-core.c 
  b/drivers/media/video/s5p-fimc/fimc-core.c
  index aa55066..7ca8091 100644
  --- a/drivers/media/video/s5p-fimc/fimc-core.c
  +++ b/drivers/media/video/s5p-fimc/fimc-core.c
  @@ -18,6 +18,7 @@
#includelinux/interrupt.h
#includelinux/device.h
#includelinux/platform_device.h
  +#includelinux/pm_runtime.h
#includelinux/list.h
#includelinux/io.h
#includelinux/slab.h
  @@ -301,7 +302,6 @@ int fimc_set_scaler_info(struct fimc_ctx *ctx)
static void fimc_m2m_job_finish(struct fimc_ctx *ctx, int vb_state)
{
 struct vb2_buffer *src_vb, *dst_vb;
  -  struct fimc_dev *fimc = ctx-fimc_dev;
 
 if (!ctx || !ctx-m2m_ctx)
 return;
  @@ -312,39 +312,48 @@ static void fimc_m2m_job_finish(struct fimc_ctx 
  *ctx, int vb_state)
 if (src_vb  dst_vb) {
 v4l2_m2m_buf_done(src_vb, vb_state);
 v4l2_m2m_buf_done(dst_vb, vb_state);
  -  v4l2_m2m_job_finish(fimc-m2m.m2m_dev, ctx-m2m_ctx);
  +  v4l2_m2m_job_finish(ctx-fimc_dev-m2m.m2m_dev,
  +  ctx-m2m_ctx);
 }
}
 
/* Complete the transaction which has been scheduled for execution. */
  -static void fimc_m2m_shutdown(struct fimc_ctx *ctx)
  +static int fimc_m2m_shutdown(struct fimc_ctx *ctx)
{
 struct fimc_dev *fimc = ctx-fimc_dev;
 int ret;
 
 if (!fimc_m2m_pending(fimc))
  -  return;
  +  return 0;
 
 fimc_ctx_state_lock_set(FIMC_CTX_SHUT, ctx);
 
 ret = wait_event_timeout(fimc-irq_queue,
!fimc_ctx_state_is_set(FIMC_CTX_SHUT, ctx),
FIMC_SHUTDOWN_TIMEOUT);
  -  /*
  -   * In case of a timeout the buffers are not released in the interrupt
  -   * handler so return them here with the error flag set, if there are
  -   * any on the queue.
  -   */
  -  if (ret == 0)
  -  fimc_m2m_job_finish(ctx, VB2_BUF_STATE_ERROR);
  +
  +  return ret == 0 ? -ETIMEDOUT : ret;