Re: [PATCH v6 05/17] media: rkisp1: add Rockchip ISP1 subdev driver

[Tomasz Figa]

On Thu, May 24, 2018 at 8:30 PM Baruch Siach  wrote:

> Hi Tomasz,

> On Mon, May 07, 2018 at 06:41:50AM +, Tomasz Figa wrote:
> > On Mon, May 7, 2018 at 3:38 PM Baruch Siach  wrote:
> > > On Mon, May 07, 2018 at 06:13:27AM +, Tomasz Figa wrote:
> > > > On Thu, May 3, 2018 at 6:09 PM Baruch Siach 
wrote:
> > > > > On Thu, Mar 08, 2018 at 05:47:55PM +0800, Jacob Chen wrote:
> > > > > > +static int rkisp1_isp_sd_s_power(struct v4l2_subdev *sd, int
on)
> > > > > > +{
> > > > > > + struct rkisp1_device *isp_dev = sd_to_isp_dev(sd);
> > > > > > + int ret;
> > > > > > +
> > > > > > + v4l2_dbg(1, rkisp1_debug, _dev->v4l2_dev, "s_power:
%d\n",
> > > > on);
> > > > > > +
> > > > > > + if (on) {
> > > > > > + ret = pm_runtime_get_sync(isp_dev->dev);
> > > > > > + if (ret < 0)
> > > > > > + return ret;
> > > > > > +
> > > > > > + rkisp1_config_clk(isp_dev);
> > > > > > + } else {
> > > > > > + ret = pm_runtime_put(isp_dev->dev);
> > > >
> > > > > I commented this line out to make more than one STREAMON work.
> > Otherwise,
> > > > > the second STREAMON hangs. I guess the bug is not this driver.
> > Probably
> > > > > something in drivers/soc/rockchip/pm_domains.c. Just noting that
in
> > case
> > > > > you or someone on Cc would like to investigate it further.
> > > > >
> > > > > I tested v4.16-rc4 on the Tinkerboard.
> > > >
> > > > Looks like that version doesn't include the IOMMU PM and clock
handling
> > > > rework [1], which should fix a lot of runtime PM issues. FWIW,
> > linux-next
> > > > seems to already include it.
> > > >
> > > > [1] https://lkml.org/lkml/2018/3/23/44
> >
> > > Thanks for the reference.
> >
> > > It looks like the iommu driver part is in Linus' tree already. The DT
> > part is
> > > in the v4.18-armsoc/dts32 branch of Heiko's tree. Am I missing
anything?
> >
> > You're right, most of the series made it in time for 4.17. However, the
DT
> > part (precisely, the clocks properties added to IOMMU nodes) is crucial
for
> > the fixes to be effective.
> >
> > > Anyway, I'll take a look.
> >
> > Thanks for testing. :) (Forgot to mention in my previous email...)

> I finally got around to testing. Unfortunately, kernel v4.17-rc6 with
> cherry-pick of commit c78751f91c0b (ARM: dts: rockchip: add clocks in
iommu
> nodes) from Heiko's tree still exhibit the same problem. STREAMON hangs on
> second try. The same workaround "fixes" it.

Thanks for testing. I'm out of ideas, since the same code seems to work
fine for us in Chrome OS 4.4 kernel. Maybe we could have someone from RK
take a look.

Best regards,
Tomasz

Re: [PATCH v6 05/17] media: rkisp1: add Rockchip ISP1 subdev driver

[Baruch Siach]

Hi Tomasz,

On Mon, May 07, 2018 at 06:41:50AM +, Tomasz Figa wrote:
> On Mon, May 7, 2018 at 3:38 PM Baruch Siach  wrote:
> > On Mon, May 07, 2018 at 06:13:27AM +, Tomasz Figa wrote:
> > > On Thu, May 3, 2018 at 6:09 PM Baruch Siach  wrote:
> > > > On Thu, Mar 08, 2018 at 05:47:55PM +0800, Jacob Chen wrote:
> > > > > +static int rkisp1_isp_sd_s_power(struct v4l2_subdev *sd, int on)
> > > > > +{
> > > > > + struct rkisp1_device *isp_dev = sd_to_isp_dev(sd);
> > > > > + int ret;
> > > > > +
> > > > > + v4l2_dbg(1, rkisp1_debug, _dev->v4l2_dev, "s_power: %d\n",
> > > on);
> > > > > +
> > > > > + if (on) {
> > > > > + ret = pm_runtime_get_sync(isp_dev->dev);
> > > > > + if (ret < 0)
> > > > > + return ret;
> > > > > +
> > > > > + rkisp1_config_clk(isp_dev);
> > > > > + } else {
> > > > > + ret = pm_runtime_put(isp_dev->dev);
> > >
> > > > I commented this line out to make more than one STREAMON work.
> Otherwise,
> > > > the second STREAMON hangs. I guess the bug is not this driver.
> Probably
> > > > something in drivers/soc/rockchip/pm_domains.c. Just noting that in
> case
> > > > you or someone on Cc would like to investigate it further.
> > > >
> > > > I tested v4.16-rc4 on the Tinkerboard.
> > >
> > > Looks like that version doesn't include the IOMMU PM and clock handling
> > > rework [1], which should fix a lot of runtime PM issues. FWIW,
> linux-next
> > > seems to already include it.
> > >
> > > [1] https://lkml.org/lkml/2018/3/23/44
> 
> > Thanks for the reference.
> 
> > It looks like the iommu driver part is in Linus' tree already. The DT
> part is
> > in the v4.18-armsoc/dts32 branch of Heiko's tree. Am I missing anything?
> 
> You're right, most of the series made it in time for 4.17. However, the DT
> part (precisely, the clocks properties added to IOMMU nodes) is crucial for
> the fixes to be effective.
> 
> > Anyway, I'll take a look.
> 
> Thanks for testing. :) (Forgot to mention in my previous email...)

I finally got around to testing. Unfortunately, kernel v4.17-rc6 with 
cherry-pick of commit c78751f91c0b (ARM: dts: rockchip: add clocks in iommu 
nodes) from Heiko's tree still exhibit the same problem. STREAMON hangs on 
second try. The same workaround "fixes" it.

baruch

-- 
 http://baruch.siach.name/blog/  ~. .~   Tk Open Systems
=}ooO--U--Ooo{=
   - bar...@tkos.co.il - tel: +972.52.368.4656, http://www.tkos.co.il -

Re: [PATCH v6 05/17] media: rkisp1: add Rockchip ISP1 subdev driver

[Tomasz Figa]

On Mon, May 7, 2018 at 3:38 PM Baruch Siach  wrote:

> Hi Tomasz,

> On Mon, May 07, 2018 at 06:13:27AM +, Tomasz Figa wrote:
> > On Thu, May 3, 2018 at 6:09 PM Baruch Siach  wrote:
> > > On Thu, Mar 08, 2018 at 05:47:55PM +0800, Jacob Chen wrote:
> > > > +static int rkisp1_isp_sd_s_power(struct v4l2_subdev *sd, int on)
> > > > +{
> > > > + struct rkisp1_device *isp_dev = sd_to_isp_dev(sd);
> > > > + int ret;
> > > > +
> > > > + v4l2_dbg(1, rkisp1_debug, _dev->v4l2_dev, "s_power: %d\n",
> > on);
> > > > +
> > > > + if (on) {
> > > > + ret = pm_runtime_get_sync(isp_dev->dev);
> > > > + if (ret < 0)
> > > > + return ret;
> > > > +
> > > > + rkisp1_config_clk(isp_dev);
> > > > + } else {
> > > > + ret = pm_runtime_put(isp_dev->dev);
> >
> > > I commented this line out to make more than one STREAMON work.
Otherwise,
> > > the second STREAMON hangs. I guess the bug is not this driver.
Probably
> > > something in drivers/soc/rockchip/pm_domains.c. Just noting that in
case
> > > you or someone on Cc would like to investigate it further.
> > >
> > > I tested v4.16-rc4 on the Tinkerboard.
> >
> > Looks like that version doesn't include the IOMMU PM and clock handling
> > rework [1], which should fix a lot of runtime PM issues. FWIW,
linux-next
> > seems to already include it.
> >
> > [1] https://lkml.org/lkml/2018/3/23/44

> Thanks for the reference.

> It looks like the iommu driver part is in Linus' tree already. The DT
part is
> in the v4.18-armsoc/dts32 branch of Heiko's tree. Am I missing anything?

You're right, most of the series made it in time for 4.17. However, the DT
part (precisely, the clocks properties added to IOMMU nodes) is crucial for
the fixes to be effective.


> Anyway, I'll take a look.

Thanks for testing. :) (Forgot to mention in my previous email...)

Best regards,
Tomasz

Re: [PATCH v6 05/17] media: rkisp1: add Rockchip ISP1 subdev driver

[Baruch Siach]

Hi Tomasz,

On Mon, May 07, 2018 at 06:13:27AM +, Tomasz Figa wrote:
> On Thu, May 3, 2018 at 6:09 PM Baruch Siach  wrote:
> > On Thu, Mar 08, 2018 at 05:47:55PM +0800, Jacob Chen wrote:
> > > +static int rkisp1_isp_sd_s_power(struct v4l2_subdev *sd, int on)
> > > +{
> > > + struct rkisp1_device *isp_dev = sd_to_isp_dev(sd);
> > > + int ret;
> > > +
> > > + v4l2_dbg(1, rkisp1_debug, _dev->v4l2_dev, "s_power: %d\n",
> on);
> > > +
> > > + if (on) {
> > > + ret = pm_runtime_get_sync(isp_dev->dev);
> > > + if (ret < 0)
> > > + return ret;
> > > +
> > > + rkisp1_config_clk(isp_dev);
> > > + } else {
> > > + ret = pm_runtime_put(isp_dev->dev);
> 
> > I commented this line out to make more than one STREAMON work. Otherwise, 
> > the second STREAMON hangs. I guess the bug is not this driver. Probably
> > something in drivers/soc/rockchip/pm_domains.c. Just noting that in case 
> > you or someone on Cc would like to investigate it further.
> >
> > I tested v4.16-rc4 on the Tinkerboard.
> 
> Looks like that version doesn't include the IOMMU PM and clock handling
> rework [1], which should fix a lot of runtime PM issues. FWIW, linux-next
> seems to already include it.
> 
> [1] https://lkml.org/lkml/2018/3/23/44

Thanks for the reference.

It looks like the iommu driver part is in Linus' tree already. The DT part is 
in the v4.18-armsoc/dts32 branch of Heiko's tree. Am I missing anything?

Anyway, I'll take a look.

Thanks again,
baruch

-- 
 http://baruch.siach.name/blog/  ~. .~   Tk Open Systems
=}ooO--U--Ooo{=
   - bar...@tkos.co.il - tel: +972.52.368.4656, http://www.tkos.co.il -

Re: [PATCH v6 05/17] media: rkisp1: add Rockchip ISP1 subdev driver

[Tomasz Figa]

Hi Baruch,

On Thu, May 3, 2018 at 6:09 PM Baruch Siach  wrote:

> Hi Jacob,

> On Thu, Mar 08, 2018 at 05:47:55PM +0800, Jacob Chen wrote:
> > +static int rkisp1_isp_sd_s_power(struct v4l2_subdev *sd, int on)
> > +{
> > + struct rkisp1_device *isp_dev = sd_to_isp_dev(sd);
> > + int ret;
> > +
> > + v4l2_dbg(1, rkisp1_debug, _dev->v4l2_dev, "s_power: %d\n",
on);
> > +
> > + if (on) {
> > + ret = pm_runtime_get_sync(isp_dev->dev);
> > + if (ret < 0)
> > + return ret;
> > +
> > + rkisp1_config_clk(isp_dev);
> > + } else {
> > + ret = pm_runtime_put(isp_dev->dev);

> I commented this line out to make more than one STREAMON work. Otherwise,
the
> second STREAMON hangs. I guess the bug is not this driver. Probably
something
> in drivers/soc/rockchip/pm_domains.c. Just noting that in case you or
someone
> on Cc would like to investigate it further.

> I tested v4.16-rc4 on the Tinkerboard.

Looks like that version doesn't include the IOMMU PM and clock handling
rework [1], which should fix a lot of runtime PM issues. FWIW, linux-next
seems to already include it.

[1] https://lkml.org/lkml/2018/3/23/44

Best regards,
Tomasz

Re: [PATCH v6 05/17] media: rkisp1: add Rockchip ISP1 subdev driver

[Baruch Siach]

Hi Jacob,

On Thu, Mar 08, 2018 at 05:47:55PM +0800, Jacob Chen wrote:
> +static int rkisp1_isp_sd_s_power(struct v4l2_subdev *sd, int on)
> +{
> + struct rkisp1_device *isp_dev = sd_to_isp_dev(sd);
> + int ret;
> +
> + v4l2_dbg(1, rkisp1_debug, _dev->v4l2_dev, "s_power: %d\n", on);
> +
> + if (on) {
> + ret = pm_runtime_get_sync(isp_dev->dev);
> + if (ret < 0)
> + return ret;
> +
> + rkisp1_config_clk(isp_dev);
> + } else {
> + ret = pm_runtime_put(isp_dev->dev);

I commented this line out to make more than one STREAMON work. Otherwise, the 
second STREAMON hangs. I guess the bug is not this driver. Probably something 
in drivers/soc/rockchip/pm_domains.c. Just noting that in case you or someone 
on Cc would like to investigate it further.

I tested v4.16-rc4 on the Tinkerboard.

baruch

> + if (ret < 0)
> + return ret;
> + }
> +
> + return 0;
> +}

-- 
 http://baruch.siach.name/blog/  ~. .~   Tk Open Systems
=}ooO--U--Ooo{=
   - bar...@tkos.co.il - tel: +972.52.368.4656, http://www.tkos.co.il -

Re: [PATCH v6 05/17] media: rkisp1: add Rockchip ISP1 subdev driver

[Hans Verkuil]

On 08/03/18 10:47, Jacob Chen wrote:
> From: Jacob Chen 
> 
> Add the subdev driver for rockchip isp1.
> 
> Signed-off-by: Jacob Chen 
> Signed-off-by: Shunqian Zheng 
> Signed-off-by: Yichong Zhong 
> Signed-off-by: Jacob Chen 
> Signed-off-by: Eddie Cai 
> Signed-off-by: Jeffy Chen 
> Signed-off-by: Allon Huang 
> Signed-off-by: Tomasz Figa 
> ---
>  drivers/media/platform/rockchip/isp1/rkisp1.c | 1177 
> +
>  drivers/media/platform/rockchip/isp1/rkisp1.h |  105 +++
>  2 files changed, 1282 insertions(+)
>  create mode 100644 drivers/media/platform/rockchip/isp1/rkisp1.c
>  create mode 100644 drivers/media/platform/rockchip/isp1/rkisp1.h
> 
> diff --git a/drivers/media/platform/rockchip/isp1/rkisp1.c 
> b/drivers/media/platform/rockchip/isp1/rkisp1.c
> new file mode 100644
> index ..bb16c8118c16
> --- /dev/null
> +++ b/drivers/media/platform/rockchip/isp1/rkisp1.c
> @@ -0,0 +1,1177 @@
> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> +/*
> + * Rockchip isp1 driver
> + *
> + * Copyright (C) 2017 Rockchip Electronics Co., Ltd.
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include "common.h"
> +#include "regs.h"
> +
> +#define CIF_ISP_INPUT_W_MAX  4032
> +#define CIF_ISP_INPUT_H_MAX  3024
> +#define CIF_ISP_INPUT_W_MIN  32
> +#define CIF_ISP_INPUT_H_MIN  32
> +#define CIF_ISP_OUTPUT_W_MAX CIF_ISP_INPUT_W_MAX
> +#define CIF_ISP_OUTPUT_H_MAX CIF_ISP_INPUT_H_MAX
> +#define CIF_ISP_OUTPUT_W_MIN CIF_ISP_INPUT_W_MIN
> +#define CIF_ISP_OUTPUT_H_MIN CIF_ISP_INPUT_H_MIN
> +
> +/*
> + * NOTE: MIPI controller and input MUX are also configured in this file,
> + * because ISP Subdev is not only describe ISP submodule(input size,format, 
> output size, format),
> + * but also a virtual route device.
> + */
> +
> +/*
> + * There are many variables named with format/frame in below code,
> + * please see here for their meaning.
> + *
> + * Cropping regions of ISP
> + *
> + * +-+
> + * | Sensor image|
> + * | +---+   |
> + * | | ISP_ACQ (for black level) |   |
> + * | | in_frm|   |
> + * | | ++|   |
> + * | | |ISP_OUT ||   |
> + * | | |in_crop ||   |
> + * | | |+-+ ||   |
> + * | | ||   ISP_IS| ||   |
> + * | | ||   rkisp1_isp_subdev: out_crop   | ||   |
> + * | | |+-+ ||   |
> + * | | ++|   |
> + * | +---+   |
> + * +-+
> + */
> +
> +static inline struct rkisp1_device *sd_to_isp_dev(struct v4l2_subdev *sd)
> +{
> + return container_of(sd->v4l2_dev, struct rkisp1_device, v4l2_dev);
> +}
> +
> +/* Get sensor by enabled media link */
> +static struct v4l2_subdev *get_remote_sensor(struct v4l2_subdev *sd)
> +{
> + struct media_pad *local;
> + struct media_entity *sensor_me;
> +
> + local = >entity.pads[RKISP1_ISP_PAD_SINK];
> + sensor_me = media_entity_remote_pad(local)->entity;
> +
> + return media_entity_to_v4l2_subdev(sensor_me);
> +}
> +
> +static struct rkisp1_sensor_info *sd_to_sensor(struct rkisp1_device *dev,
> +struct v4l2_subdev *sd)
> +{
> + int i;
> +
> + for (i = 0; i < dev->num_sensors; ++i)
> + if (dev->sensors[i].sd == sd)
> + return >sensors[i];
> +
> + return NULL;
> +}
> +
> +/  register operations /
> +
> +/*
> + * Image Stabilization.
> + * This should only be called when configuring CIF
> + * or at the frame end interrupt
> + */
> +static void rkisp1_config_ism(struct rkisp1_device *dev)
> +{
> + void __iomem *base = dev->base_addr;
> + struct v4l2_rect *out_crop = >isp_sdev.out_crop;
> + u32 val;
> +
> + writel(0, base + CIF_ISP_IS_RECENTER);
> + writel(0, base + CIF_ISP_IS_MAX_DX);
> + writel(0, base + CIF_ISP_IS_MAX_DY);
> + writel(0, base + CIF_ISP_IS_DISPLACE);
> + writel(out_crop->left, base + CIF_ISP_IS_H_OFFS);
> + writel(out_crop->top, base + CIF_ISP_IS_V_OFFS);
> + writel(out_crop->width, base + CIF_ISP_IS_H_SIZE);
> + writel(out_crop->height, base + CIF_ISP_IS_V_SIZE);
> +
> + /* IS(Image Stabilization) is always on, working as 

[PATCH v6 05/17] media: rkisp1: add Rockchip ISP1 subdev driver

[Jacob Chen]

From: Jacob Chen 

Add the subdev driver for rockchip isp1.

Signed-off-by: Jacob Chen 
Signed-off-by: Shunqian Zheng 
Signed-off-by: Yichong Zhong 
Signed-off-by: Jacob Chen 
Signed-off-by: Eddie Cai 
Signed-off-by: Jeffy Chen 
Signed-off-by: Allon Huang 
Signed-off-by: Tomasz Figa 
---
 drivers/media/platform/rockchip/isp1/rkisp1.c | 1177 +
 drivers/media/platform/rockchip/isp1/rkisp1.h |  105 +++
 2 files changed, 1282 insertions(+)
 create mode 100644 drivers/media/platform/rockchip/isp1/rkisp1.c
 create mode 100644 drivers/media/platform/rockchip/isp1/rkisp1.h

diff --git a/drivers/media/platform/rockchip/isp1/rkisp1.c 
b/drivers/media/platform/rockchip/isp1/rkisp1.c
new file mode 100644
index ..bb16c8118c16
--- /dev/null
+++ b/drivers/media/platform/rockchip/isp1/rkisp1.c
@@ -0,0 +1,1177 @@
+// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
+/*
+ * Rockchip isp1 driver
+ *
+ * Copyright (C) 2017 Rockchip Electronics Co., Ltd.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include "common.h"
+#include "regs.h"
+
+#define CIF_ISP_INPUT_W_MAX4032
+#define CIF_ISP_INPUT_H_MAX3024
+#define CIF_ISP_INPUT_W_MIN32
+#define CIF_ISP_INPUT_H_MIN32
+#define CIF_ISP_OUTPUT_W_MAX   CIF_ISP_INPUT_W_MAX
+#define CIF_ISP_OUTPUT_H_MAX   CIF_ISP_INPUT_H_MAX
+#define CIF_ISP_OUTPUT_W_MIN   CIF_ISP_INPUT_W_MIN
+#define CIF_ISP_OUTPUT_H_MIN   CIF_ISP_INPUT_H_MIN
+
+/*
+ * NOTE: MIPI controller and input MUX are also configured in this file,
+ * because ISP Subdev is not only describe ISP submodule(input size,format, 
output size, format),
+ * but also a virtual route device.
+ */
+
+/*
+ * There are many variables named with format/frame in below code,
+ * please see here for their meaning.
+ *
+ * Cropping regions of ISP
+ *
+ * +-+
+ * | Sensor image|
+ * | +---+   |
+ * | | ISP_ACQ (for black level) |   |
+ * | | in_frm|   |
+ * | | ++|   |
+ * | | |ISP_OUT ||   |
+ * | | |in_crop ||   |
+ * | | |+-+ ||   |
+ * | | ||   ISP_IS| ||   |
+ * | | ||   rkisp1_isp_subdev: out_crop   | ||   |
+ * | | |+-+ ||   |
+ * | | ++|   |
+ * | +---+   |
+ * +-+
+ */
+
+static inline struct rkisp1_device *sd_to_isp_dev(struct v4l2_subdev *sd)
+{
+   return container_of(sd->v4l2_dev, struct rkisp1_device, v4l2_dev);
+}
+
+/* Get sensor by enabled media link */
+static struct v4l2_subdev *get_remote_sensor(struct v4l2_subdev *sd)
+{
+   struct media_pad *local;
+   struct media_entity *sensor_me;
+
+   local = >entity.pads[RKISP1_ISP_PAD_SINK];
+   sensor_me = media_entity_remote_pad(local)->entity;
+
+   return media_entity_to_v4l2_subdev(sensor_me);
+}
+
+static struct rkisp1_sensor_info *sd_to_sensor(struct rkisp1_device *dev,
+  struct v4l2_subdev *sd)
+{
+   int i;
+
+   for (i = 0; i < dev->num_sensors; ++i)
+   if (dev->sensors[i].sd == sd)
+   return >sensors[i];
+
+   return NULL;
+}
+
+/  register operations /
+
+/*
+ * Image Stabilization.
+ * This should only be called when configuring CIF
+ * or at the frame end interrupt
+ */
+static void rkisp1_config_ism(struct rkisp1_device *dev)
+{
+   void __iomem *base = dev->base_addr;
+   struct v4l2_rect *out_crop = >isp_sdev.out_crop;
+   u32 val;
+
+   writel(0, base + CIF_ISP_IS_RECENTER);
+   writel(0, base + CIF_ISP_IS_MAX_DX);
+   writel(0, base + CIF_ISP_IS_MAX_DY);
+   writel(0, base + CIF_ISP_IS_DISPLACE);
+   writel(out_crop->left, base + CIF_ISP_IS_H_OFFS);
+   writel(out_crop->top, base + CIF_ISP_IS_V_OFFS);
+   writel(out_crop->width, base + CIF_ISP_IS_H_SIZE);
+   writel(out_crop->height, base + CIF_ISP_IS_V_SIZE);
+
+   /* IS(Image Stabilization) is always on, working as output crop */
+   writel(1, base + CIF_ISP_IS_CTRL);
+   val = readl(base + CIF_ISP_CTRL);
+   val |= CIF_ISP_CTRL_ISP_CFG_UPD;
+   writel(val, base + CIF_ISP_CTRL);
+}
+
+/*
+ * configure isp blocks with input format,