Re: [PATCH v2 1/3] media: V3s: Add support for Allwinner CSI.

[Maxime Ripard]

Hi,

On Fri, Dec 15, 2017 at 07:01:40PM +0800, Yong wrote:
> Hi Maxime,
> 
> On Fri, 15 Dec 2017 11:50:47 +0100
> Maxime Ripard  wrote:
> 
> > Hi Yong,
> > 
> > On Mon, Dec 04, 2017 at 05:45:11PM +0800, Yong wrote:
> > > I just noticed that you are using the second iteration?
> > > Have you received my third iteration?
> > 
> > Sorry for the late reply, and for not coming back to you yet about
> > that test. No, this is still in your v2. I've definitely received your
> > v3, I just didn't have time to update to it yet.
> > 
> > But don't worry, my mail was mostly to know if you had tested that
> > setup on your side to try to nail down the issue on my end, not really
> > a review or comment that would prevent your patch from going in.
> 
> I mean,
> The v2 exactly has a bug which may cause the CSI writing frame to 
> a wrong memory address.

Ah, sorry I misunderstood you then. I'll definitely test your v3..

> BTW, should I send a new version. I have made some improve sine v3.

.. or your v4 :)

Yes, please send a new version.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [PATCH v2 1/3] media: V3s: Add support for Allwinner CSI.

[Yong]

Hi Maxime,

On Fri, 15 Dec 2017 11:50:47 +0100
Maxime Ripard  wrote:

> Hi Yong,
> 
> On Mon, Dec 04, 2017 at 05:45:11PM +0800, Yong wrote:
> > I just noticed that you are using the second iteration?
> > Have you received my third iteration?
> 
> Sorry for the late reply, and for not coming back to you yet about
> that test. No, this is still in your v2. I've definitely received your
> v3, I just didn't have time to update to it yet.
> 
> But don't worry, my mail was mostly to know if you had tested that
> setup on your side to try to nail down the issue on my end, not really
> a review or comment that would prevent your patch from going in.

I mean,
The v2 exactly has a bug which may cause the CSI writing frame to 
a wrong memory address.

BTW, should I send a new version. I have made some improve sine v3.

> 
> Maxime
> 
> -- 
> Maxime Ripard, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com


Thanks,
Yong

Re: [PATCH v2 1/3] media: V3s: Add support for Allwinner CSI.

[Maxime Ripard]

Hi Yong,

On Mon, Dec 04, 2017 at 05:45:11PM +0800, Yong wrote:
> I just noticed that you are using the second iteration?
> Have you received my third iteration?

Sorry for the late reply, and for not coming back to you yet about
that test. No, this is still in your v2. I've definitely received your
v3, I just didn't have time to update to it yet.

But don't worry, my mail was mostly to know if you had tested that
setup on your side to try to nail down the issue on my end, not really
a review or comment that would prevent your patch from going in.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com


signature.asc
Description: PGP signature

Re: [PATCH v2 1/3] media: V3s: Add support for Allwinner CSI.

[Yong]

Hi Maxime,

I just noticed that you are using the second iteration?
Have you received my third iteration?

On Sat, 25 Nov 2017 17:02:33 +0100
Maxime Ripard  wrote:

> On Thu, Nov 23, 2017 at 09:14:44AM +0800, Yong wrote:
> > > On Wed, Nov 22, 2017 at 09:33:06AM +0800, Yong wrote:
> > > > > On Thu, Jul 27, 2017 at 01:01:35PM +0800, Yong Deng wrote:
> > > > > > Allwinner V3s SoC have two CSI module. CSI0 is used for MIPI 
> > > > > > interface
> > > > > > and CSI1 is used for parallel interface. This is not documented in
> > > > > > datasheet but by testing and guess.
> > > > > > 
> > > > > > This patch implement a v4l2 framework driver for it.
> > > > > > 
> > > > > > Currently, the driver only support the parallel interface. 
> > > > > > MIPI-CSI2,
> > > > > > ISP's support are not included in this patch.
> > > > > > 
> > > > > > Signed-off-by: Yong Deng 
> > > > > 
> > > > > Thanks again for this driver.
> > > > > 
> > > > > It seems like at least this iteration is behaving in a weird way with
> > > > > DMA transfers for at least YU12 and NV12 (and I would assume YV12).
> > > > > 
> > > > > Starting a transfer of multiple frames in either of these formats,
> > > > > using either ffmpeg (ffmpeg -f v4l2 -video_size 640x480 -framerate 30
> > > > > -i /dev/video0 output.mkv) or yavta (yavta -c80 -p -F --skip 0 -f NV12
> > > > > -s 640x480 $(media-c tl -e 'sun6i-csi')) will end up in a panic.
> > > > > 
> > > > > The panic seems to be generated with random data going into parts of
> > > > > the kernel memory, the pattern being in my case something like
> > > > > 0x8287868a which is very odd (always around 0x88)
> > > > > 
> > > > > It turns out that when you cover the sensor, the values change to
> > > > > around 0x28, so it really seems like it's pixels that have been copied
> > > > > there.
> > > > > 
> > > > > I've looked quickly at the DMA setup, and it seems reasonable to
> > > > > me. Do you have the same issue on your side? Have you been able to
> > > > > test those formats using your hardware?
> > > > 
> > > > I had tested the following formats with BT1120 input:
> > > > V4L2_PIX_FMT_NV12   -> NV12
> > > > V4L2_PIX_FMT_NV21   -> NV21
> > > > V4L2_PIX_FMT_NV16   -> NV16
> > > > V4L2_PIX_FMT_NV61   -> NV61
> > > > V4L2_PIX_FMT_YUV420 -> YU12
> > > > V4L2_PIX_FMT_YVU420 -> YV12
> > > > V4L2_PIX_FMT_YUV422P-> 422P
> > > > And they all work fine.
> > > 
> > > Ok, that's good to know.
> > > 
> > > > > Given that they all are planar formats and YUYV and the likes work
> > > > > just fine, maybe we can leave them aside for now?
> > > > 
> > > > V4L2_PIX_FMT_YUV422P and V4L2_PIX_FMT_YUYV is OK, and V4L2_PIX_FMT_NV12
> > > > is bad? It's really weird.
> > > > 
> > > > What's your input bus code format, type and width?
> > > 
> > > The sensor is an ov5640, so the MBUS code for the bus is
> > > MEDIA_BUS_FMT_YUYV8_2X8.
> > 
> > Did you test on V3s?
> 
> No, this is on an H3, but that would be the first difference so far.
> 
> > I haven't tested it with MEDIA_BUS_FMT_YUYV8_2X8.
> 
> Ok, it's good to know that at least it works on your end, it's useful
> for us to debug things :)
> 
> > The Allwinner CSI's DMA is definitely weird. Ondřej Jirman thought
> > that CSI has an internal queue (Ondřej's commit has explained in detail).
> > I think CSI just pick up the buffer address before the frame done 
> > interrupt triggered. 
> > The patch in attachment can deal with this. You can see if it is
> > useful to solve your problem.
> 
> I'll test that on monday, thanks!
> 
> Maxime
> 
> -- 
> Maxime Ripard, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com


Thanks,
Yong

Re: [PATCH v2 1/3] media: V3s: Add support for Allwinner CSI.

[Maxime Ripard]

On Thu, Nov 23, 2017 at 09:14:44AM +0800, Yong wrote:
> > On Wed, Nov 22, 2017 at 09:33:06AM +0800, Yong wrote:
> > > > On Thu, Jul 27, 2017 at 01:01:35PM +0800, Yong Deng wrote:
> > > > > Allwinner V3s SoC have two CSI module. CSI0 is used for MIPI interface
> > > > > and CSI1 is used for parallel interface. This is not documented in
> > > > > datasheet but by testing and guess.
> > > > > 
> > > > > This patch implement a v4l2 framework driver for it.
> > > > > 
> > > > > Currently, the driver only support the parallel interface. MIPI-CSI2,
> > > > > ISP's support are not included in this patch.
> > > > > 
> > > > > Signed-off-by: Yong Deng 
> > > > 
> > > > Thanks again for this driver.
> > > > 
> > > > It seems like at least this iteration is behaving in a weird way with
> > > > DMA transfers for at least YU12 and NV12 (and I would assume YV12).
> > > > 
> > > > Starting a transfer of multiple frames in either of these formats,
> > > > using either ffmpeg (ffmpeg -f v4l2 -video_size 640x480 -framerate 30
> > > > -i /dev/video0 output.mkv) or yavta (yavta -c80 -p -F --skip 0 -f NV12
> > > > -s 640x480 $(media-c tl -e 'sun6i-csi')) will end up in a panic.
> > > > 
> > > > The panic seems to be generated with random data going into parts of
> > > > the kernel memory, the pattern being in my case something like
> > > > 0x8287868a which is very odd (always around 0x88)
> > > > 
> > > > It turns out that when you cover the sensor, the values change to
> > > > around 0x28, so it really seems like it's pixels that have been copied
> > > > there.
> > > > 
> > > > I've looked quickly at the DMA setup, and it seems reasonable to
> > > > me. Do you have the same issue on your side? Have you been able to
> > > > test those formats using your hardware?
> > > 
> > > I had tested the following formats with BT1120 input:
> > > V4L2_PIX_FMT_NV12 -> NV12
> > > V4L2_PIX_FMT_NV21 -> NV21
> > > V4L2_PIX_FMT_NV16 -> NV16
> > > V4L2_PIX_FMT_NV61 -> NV61
> > > V4L2_PIX_FMT_YUV420   -> YU12
> > > V4L2_PIX_FMT_YVU420   -> YV12
> > > V4L2_PIX_FMT_YUV422P  -> 422P
> > > And they all work fine.
> > 
> > Ok, that's good to know.
> > 
> > > > Given that they all are planar formats and YUYV and the likes work
> > > > just fine, maybe we can leave them aside for now?
> > > 
> > > V4L2_PIX_FMT_YUV422P and V4L2_PIX_FMT_YUYV is OK, and V4L2_PIX_FMT_NV12
> > > is bad? It's really weird.
> > > 
> > > What's your input bus code format, type and width?
> > 
> > The sensor is an ov5640, so the MBUS code for the bus is
> > MEDIA_BUS_FMT_YUYV8_2X8.
> 
> Did you test on V3s?

No, this is on an H3, but that would be the first difference so far.

> I haven't tested it with MEDIA_BUS_FMT_YUYV8_2X8.

Ok, it's good to know that at least it works on your end, it's useful
for us to debug things :)

> The Allwinner CSI's DMA is definitely weird. Ondřej Jirman thought
> that CSI has an internal queue (Ondřej's commit has explained in detail).
> I think CSI just pick up the buffer address before the frame done 
> interrupt triggered. 
> The patch in attachment can deal with this. You can see if it is
> useful to solve your problem.

I'll test that on monday, thanks!

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com


signature.asc
Description: PGP signature

Re: [PATCH v2 1/3] media: V3s: Add support for Allwinner CSI.

[Yong]

Hi,

On Wed, 22 Nov 2017 10:45:26 +0100
Maxime Ripard  wrote:

> Hi,
> 
> On Wed, Nov 22, 2017 at 09:33:06AM +0800, Yong wrote:
> > > On Thu, Jul 27, 2017 at 01:01:35PM +0800, Yong Deng wrote:
> > > > Allwinner V3s SoC have two CSI module. CSI0 is used for MIPI interface
> > > > and CSI1 is used for parallel interface. This is not documented in
> > > > datasheet but by testing and guess.
> > > > 
> > > > This patch implement a v4l2 framework driver for it.
> > > > 
> > > > Currently, the driver only support the parallel interface. MIPI-CSI2,
> > > > ISP's support are not included in this patch.
> > > > 
> > > > Signed-off-by: Yong Deng 
> > > 
> > > Thanks again for this driver.
> > > 
> > > It seems like at least this iteration is behaving in a weird way with
> > > DMA transfers for at least YU12 and NV12 (and I would assume YV12).
> > > 
> > > Starting a transfer of multiple frames in either of these formats,
> > > using either ffmpeg (ffmpeg -f v4l2 -video_size 640x480 -framerate 30
> > > -i /dev/video0 output.mkv) or yavta (yavta -c80 -p -F --skip 0 -f NV12
> > > -s 640x480 $(media-c tl -e 'sun6i-csi')) will end up in a panic.
> > > 
> > > The panic seems to be generated with random data going into parts of
> > > the kernel memory, the pattern being in my case something like
> > > 0x8287868a which is very odd (always around 0x88)
> > > 
> > > It turns out that when you cover the sensor, the values change to
> > > around 0x28, so it really seems like it's pixels that have been copied
> > > there.
> > > 
> > > I've looked quickly at the DMA setup, and it seems reasonable to
> > > me. Do you have the same issue on your side? Have you been able to
> > > test those formats using your hardware?
> > 
> > I had tested the following formats with BT1120 input:
> > V4L2_PIX_FMT_NV12   -> NV12
> > V4L2_PIX_FMT_NV21   -> NV21
> > V4L2_PIX_FMT_NV16   -> NV16
> > V4L2_PIX_FMT_NV61   -> NV61
> > V4L2_PIX_FMT_YUV420 -> YU12
> > V4L2_PIX_FMT_YVU420 -> YV12
> > V4L2_PIX_FMT_YUV422P-> 422P
> > And they all work fine.
> 
> Ok, that's good to know.
> 
> > > Given that they all are planar formats and YUYV and the likes work
> > > just fine, maybe we can leave them aside for now?
> > 
> > V4L2_PIX_FMT_YUV422P and V4L2_PIX_FMT_YUYV is OK, and V4L2_PIX_FMT_NV12
> > is bad? It's really weird.
> > 
> > What's your input bus code format, type and width?
> 
> The sensor is an ov5640, so the MBUS code for the bus is
> MEDIA_BUS_FMT_YUYV8_2X8.

Did you test on V3s?
I haven't tested it with MEDIA_BUS_FMT_YUYV8_2X8.

The Allwinner CSI's DMA is definitely weird. Ondřej Jirman thought
that CSI has an internal queue (Ondřej's commit has explained in detail).
I think CSI just pick up the buffer address before the frame done 
interrupt triggered. 
The patch in attachment can deal with this. You can see if it is
useful to solve your problem.

Thanks,
Yong


sun6i_csi_fix_writing_to_incorrect_buffer.patch
Description: Binary data

Re: [PATCH v2 1/3] media: V3s: Add support for Allwinner CSI.

[Maxime Ripard]

Hi,

On Wed, Nov 22, 2017 at 09:33:06AM +0800, Yong wrote:
> > On Thu, Jul 27, 2017 at 01:01:35PM +0800, Yong Deng wrote:
> > > Allwinner V3s SoC have two CSI module. CSI0 is used for MIPI interface
> > > and CSI1 is used for parallel interface. This is not documented in
> > > datasheet but by testing and guess.
> > > 
> > > This patch implement a v4l2 framework driver for it.
> > > 
> > > Currently, the driver only support the parallel interface. MIPI-CSI2,
> > > ISP's support are not included in this patch.
> > > 
> > > Signed-off-by: Yong Deng 
> > 
> > Thanks again for this driver.
> > 
> > It seems like at least this iteration is behaving in a weird way with
> > DMA transfers for at least YU12 and NV12 (and I would assume YV12).
> > 
> > Starting a transfer of multiple frames in either of these formats,
> > using either ffmpeg (ffmpeg -f v4l2 -video_size 640x480 -framerate 30
> > -i /dev/video0 output.mkv) or yavta (yavta -c80 -p -F --skip 0 -f NV12
> > -s 640x480 $(media-c tl -e 'sun6i-csi')) will end up in a panic.
> > 
> > The panic seems to be generated with random data going into parts of
> > the kernel memory, the pattern being in my case something like
> > 0x8287868a which is very odd (always around 0x88)
> > 
> > It turns out that when you cover the sensor, the values change to
> > around 0x28, so it really seems like it's pixels that have been copied
> > there.
> > 
> > I've looked quickly at the DMA setup, and it seems reasonable to
> > me. Do you have the same issue on your side? Have you been able to
> > test those formats using your hardware?
> 
> I had tested the following formats with BT1120 input:
> V4L2_PIX_FMT_NV12 -> NV12
> V4L2_PIX_FMT_NV21 -> NV21
> V4L2_PIX_FMT_NV16 -> NV16
> V4L2_PIX_FMT_NV61 -> NV61
> V4L2_PIX_FMT_YUV420   -> YU12
> V4L2_PIX_FMT_YVU420   -> YV12
> V4L2_PIX_FMT_YUV422P  -> 422P
> And they all work fine.

Ok, that's good to know.

> > Given that they all are planar formats and YUYV and the likes work
> > just fine, maybe we can leave them aside for now?
> 
> V4L2_PIX_FMT_YUV422P and V4L2_PIX_FMT_YUYV is OK, and V4L2_PIX_FMT_NV12
> is bad? It's really weird.
> 
> What's your input bus code format, type and width?

The sensor is an ov5640, so the MBUS code for the bus is
MEDIA_BUS_FMT_YUYV8_2X8.

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com


signature.asc
Description: PGP signature

Re: [PATCH v2 1/3] media: V3s: Add support for Allwinner CSI.

[Yong]

On Tue, 21 Nov 2017 16:48:27 +0100
Maxime Ripard  wrote:

> Hi,
> 
> On Thu, Jul 27, 2017 at 01:01:35PM +0800, Yong Deng wrote:
> > Allwinner V3s SoC have two CSI module. CSI0 is used for MIPI interface
> > and CSI1 is used for parallel interface. This is not documented in
> > datasheet but by testing and guess.
> > 
> > This patch implement a v4l2 framework driver for it.
> > 
> > Currently, the driver only support the parallel interface. MIPI-CSI2,
> > ISP's support are not included in this patch.
> > 
> > Signed-off-by: Yong Deng 
> 
> Thanks again for this driver.
> 
> It seems like at least this iteration is behaving in a weird way with
> DMA transfers for at least YU12 and NV12 (and I would assume YV12).
> 
> Starting a transfer of multiple frames in either of these formats,
> using either ffmpeg (ffmpeg -f v4l2 -video_size 640x480 -framerate 30
> -i /dev/video0 output.mkv) or yavta (yavta -c80 -p -F --skip 0 -f NV12
> -s 640x480 $(media-c tl -e 'sun6i-csi')) will end up in a panic.
> 
> The panic seems to be generated with random data going into parts of
> the kernel memory, the pattern being in my case something like
> 0x8287868a which is very odd (always around 0x88)
> 
> It turns out that when you cover the sensor, the values change to
> around 0x28, so it really seems like it's pixels that have been copied
> there.
> 
> I've looked quickly at the DMA setup, and it seems reasonable to
> me. Do you have the same issue on your side? Have you been able to
> test those formats using your hardware?

I had tested the following formats with BT1120 input:
V4L2_PIX_FMT_NV12   -> NV12
V4L2_PIX_FMT_NV21   -> NV21
V4L2_PIX_FMT_NV16   -> NV16
V4L2_PIX_FMT_NV61   -> NV61
V4L2_PIX_FMT_YUV420 -> YU12
V4L2_PIX_FMT_YVU420 -> YV12
V4L2_PIX_FMT_YUV422P-> 422P
And they all work fine.

> 
> Given that they all are planar formats and YUYV and the likes work
> just fine, maybe we can leave them aside for now?

V4L2_PIX_FMT_YUV422P and V4L2_PIX_FMT_YUYV is OK, and V4L2_PIX_FMT_NV12
is bad? It's really weird.

What's your input bus code format, type and width?

> 
> Thanks!
> Maxime
> 
> -- 
> Maxime Ripard, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com


Thanks,
Yong

Re: [PATCH v2 1/3] media: V3s: Add support for Allwinner CSI.

[Maxime Ripard]

Hi,

On Thu, Jul 27, 2017 at 01:01:35PM +0800, Yong Deng wrote:
> Allwinner V3s SoC have two CSI module. CSI0 is used for MIPI interface
> and CSI1 is used for parallel interface. This is not documented in
> datasheet but by testing and guess.
> 
> This patch implement a v4l2 framework driver for it.
> 
> Currently, the driver only support the parallel interface. MIPI-CSI2,
> ISP's support are not included in this patch.
> 
> Signed-off-by: Yong Deng 

Thanks again for this driver.

It seems like at least this iteration is behaving in a weird way with
DMA transfers for at least YU12 and NV12 (and I would assume YV12).

Starting a transfer of multiple frames in either of these formats,
using either ffmpeg (ffmpeg -f v4l2 -video_size 640x480 -framerate 30
-i /dev/video0 output.mkv) or yavta (yavta -c80 -p -F --skip 0 -f NV12
-s 640x480 $(media-c tl -e 'sun6i-csi')) will end up in a panic.

The panic seems to be generated with random data going into parts of
the kernel memory, the pattern being in my case something like
0x8287868a which is very odd (always around 0x88)

It turns out that when you cover the sensor, the values change to
around 0x28, so it really seems like it's pixels that have been copied
there.

I've looked quickly at the DMA setup, and it seems reasonable to
me. Do you have the same issue on your side? Have you been able to
test those formats using your hardware?

Given that they all are planar formats and YUYV and the likes work
just fine, maybe we can leave them aside for now?

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com


signature.asc
Description: PGP signature

Re: [PATCH v2 1/3] media: V3s: Add support for Allwinner CSI.

[Yong]

Hello Mylène,

On Fri, 22 Sep 2017 10:44:13 +0200
Mylene JOSSERAND  wrote:

> Hello Yong,
> 
> Thank you for these drivers!
> 
> I tested it with an OV5640 camera on an Nanopi M1 plus (Allwinner H3)
> and I noticed that I got a frame correctly displayed only on a half of
> the frame's size.
> 
> It is related to your "sun6i_csi_set_window" function (see
> below).
> 
> > Allwinner V3s SoC have two CSI module. CSI0 is used for MIPI interface
> > and CSI1 is used for parallel interface. This is not documented in
> > datasheet but by testing and guess.
> > 
> > This patch implement a v4l2 framework driver for it.
> > 
> > Currently, the driver only support the parallel interface. MIPI-CSI2,
> > ISP's support are not included in this patch.
> > 
> > Signed-off-by: Yong Deng 
> > ---
> >  drivers/media/platform/Kconfig   |   1 +
> >  drivers/media/platform/Makefile  |   2 +
> >  drivers/media/platform/sun6i-csi/Kconfig |   9 +
> >  drivers/media/platform/sun6i-csi/Makefile|   3 +
> >  drivers/media/platform/sun6i-csi/sun6i_csi.c | 545
> > +++ drivers/media/platform/sun6i-csi/sun6i_csi.h |
> > 203 ++ drivers/media/platform/sun6i-csi/sun6i_csi_v3s.c | 827
> > +++
> > drivers/media/platform/sun6i-csi/sun6i_csi_v3s.h | 206 ++
> > drivers/media/platform/sun6i-csi/sun6i_video.c   | 663
> > ++ drivers/media/platform/sun6i-csi/sun6i_video.h
> > |  61 ++ 10 files changed, 2520 insertions(+) create mode 100644
> > drivers/media/platform/sun6i-csi/Kconfig create mode 100644
> > drivers/media/platform/sun6i-csi/Makefile create mode 100644
> > drivers/media/platform/sun6i-csi/sun6i_csi.c create mode 100644
> > drivers/media/platform/sun6i-csi/sun6i_csi.h create mode 100644
> > drivers/media/platform/sun6i-csi/sun6i_csi_v3s.c create mode 100644
> > drivers/media/platform/sun6i-csi/sun6i_csi_v3s.h create mode 100644
> > drivers/media/platform/sun6i-csi/sun6i_video.c create mode 100644
> > drivers/media/platform/sun6i-csi/sun6i_video.h
> > 
> > diff --git a/drivers/media/platform/Kconfig
> > b/drivers/media/platform/Kconfig index 0c741d1..8371a87 100644
> > --- a/drivers/media/platform/Kconfig
> > +++ b/drivers/media/platform/Kconfig
> > @@ -143,6 +143,7 @@ source "drivers/media/platform/am437x/Kconfig"
> >  source "drivers/media/platform/xilinx/Kconfig"
> >  source "drivers/media/platform/rcar-vin/Kconfig"
> >  source "drivers/media/platform/atmel/Kconfig"
> > +source "drivers/media/platform/sun6i-csi/Kconfig"
> >  
> 
> 
> 
> > +static void sun6i_csi_set_format(struct sun6i_csi_dev *sdev)
> > +{
> > +   struct sun6i_csi *csi = >csi;
> > +   u32 cfg;
> > +   u32 val;
> > +
> > +   regmap_read(sdev->regmap, CSI_CH_CFG_REG, );
> > +
> > +   cfg &= ~(CSI_CH_CFG_INPUT_FMT_MASK |
> > +CSI_CH_CFG_OUTPUT_FMT_MASK | CSI_CH_CFG_VFLIP_EN |
> > +CSI_CH_CFG_HFLIP_EN | CSI_CH_CFG_FIELD_SEL_MASK |
> > +CSI_CH_CFG_INPUT_SEQ_MASK);
> > +
> > +   val = get_csi_input_format(csi->config.code,
> > csi->config.pixelformat);
> > +   cfg |= CSI_CH_CFG_INPUT_FMT(val);
> > +
> > +   val = get_csi_output_format(csi->config.code,
> > csi->config.field);
> > +   cfg |= CSI_CH_CFG_OUTPUT_FMT(val);
> > +
> > +   val = get_csi_input_seq(csi->config.code,
> > csi->config.pixelformat);
> > +   cfg |= CSI_CH_CFG_INPUT_SEQ(val);
> > +
> > +   if (csi->config.field == V4L2_FIELD_TOP)
> > +   cfg |= CSI_CH_CFG_FIELD_SEL_FIELD0;
> > +   else if (csi->config.field == V4L2_FIELD_BOTTOM)
> > +   cfg |= CSI_CH_CFG_FIELD_SEL_FIELD1;
> > +   else
> > +   cfg |= CSI_CH_CFG_FIELD_SEL_BOTH;
> > +
> > +   regmap_write(sdev->regmap, CSI_CH_CFG_REG, cfg);
> > +}
> > +
> > +static void sun6i_csi_set_window(struct sun6i_csi_dev *sdev)
> > +{
> > +   struct sun6i_csi_config *config = >csi.config;
> > +   u32 bytesperline_y;
> > +   u32 bytesperline_c;
> > +   int *planar_offset = sdev->planar_offset;
> > +
> > +   regmap_write(sdev->regmap, CSI_CH_HSIZE_REG,
> > +CSI_CH_HSIZE_HOR_LEN(config->width) |
> > +CSI_CH_HSIZE_HOR_START(0));
> > +   regmap_write(sdev->regmap, CSI_CH_VSIZE_REG,
> > +CSI_CH_VSIZE_VER_LEN(config->height) |
> > +CSI_CH_VSIZE_VER_START(0));
> 
> In my case, the HOR_LEN and VER_LEN were not correctly configured.
> 
> They were configured to width and height (640 * 480) but as I was
> using a YUYV format, the pixel's size is 2 bytes so the
> horizontal/vertical lines' lengths were divided by 2.
> 
> Currently, I fixed that by getting the number of bytes per pixel with
> "v4l2_pixformat_get_bpp()":
> 
> +   int bytes_pixel;
> +
> +   bytes_pixel = v4l2_pixformat_get_bpp(config->pixelformat) / 8;
>  
> regmap_write(sdev->regmap, CSI_CH_HSIZE_REG,
> -CSI_CH_HSIZE_HOR_LEN(config->width) |
> +

Re: [PATCH v2 1/3] media: V3s: Add support for Allwinner CSI.

[Mylene JOSSERAND]

Hello Yong,

Thank you for these drivers!

I tested it with an OV5640 camera on an Nanopi M1 plus (Allwinner H3)
and I noticed that I got a frame correctly displayed only on a half of
the frame's size.

It is related to your "sun6i_csi_set_window" function (see
below).

> Allwinner V3s SoC have two CSI module. CSI0 is used for MIPI interface
> and CSI1 is used for parallel interface. This is not documented in
> datasheet but by testing and guess.
> 
> This patch implement a v4l2 framework driver for it.
> 
> Currently, the driver only support the parallel interface. MIPI-CSI2,
> ISP's support are not included in this patch.
> 
> Signed-off-by: Yong Deng 
> ---
>  drivers/media/platform/Kconfig   |   1 +
>  drivers/media/platform/Makefile  |   2 +
>  drivers/media/platform/sun6i-csi/Kconfig |   9 +
>  drivers/media/platform/sun6i-csi/Makefile|   3 +
>  drivers/media/platform/sun6i-csi/sun6i_csi.c | 545
> +++ drivers/media/platform/sun6i-csi/sun6i_csi.h |
> 203 ++ drivers/media/platform/sun6i-csi/sun6i_csi_v3s.c | 827
> +++
> drivers/media/platform/sun6i-csi/sun6i_csi_v3s.h | 206 ++
> drivers/media/platform/sun6i-csi/sun6i_video.c   | 663
> ++ drivers/media/platform/sun6i-csi/sun6i_video.h
> |  61 ++ 10 files changed, 2520 insertions(+) create mode 100644
> drivers/media/platform/sun6i-csi/Kconfig create mode 100644
> drivers/media/platform/sun6i-csi/Makefile create mode 100644
> drivers/media/platform/sun6i-csi/sun6i_csi.c create mode 100644
> drivers/media/platform/sun6i-csi/sun6i_csi.h create mode 100644
> drivers/media/platform/sun6i-csi/sun6i_csi_v3s.c create mode 100644
> drivers/media/platform/sun6i-csi/sun6i_csi_v3s.h create mode 100644
> drivers/media/platform/sun6i-csi/sun6i_video.c create mode 100644
> drivers/media/platform/sun6i-csi/sun6i_video.h
> 
> diff --git a/drivers/media/platform/Kconfig
> b/drivers/media/platform/Kconfig index 0c741d1..8371a87 100644
> --- a/drivers/media/platform/Kconfig
> +++ b/drivers/media/platform/Kconfig
> @@ -143,6 +143,7 @@ source "drivers/media/platform/am437x/Kconfig"
>  source "drivers/media/platform/xilinx/Kconfig"
>  source "drivers/media/platform/rcar-vin/Kconfig"
>  source "drivers/media/platform/atmel/Kconfig"
> +source "drivers/media/platform/sun6i-csi/Kconfig"
>  



> +static void sun6i_csi_set_format(struct sun6i_csi_dev *sdev)
> +{
> + struct sun6i_csi *csi = >csi;
> + u32 cfg;
> + u32 val;
> +
> + regmap_read(sdev->regmap, CSI_CH_CFG_REG, );
> +
> + cfg &= ~(CSI_CH_CFG_INPUT_FMT_MASK |
> +  CSI_CH_CFG_OUTPUT_FMT_MASK | CSI_CH_CFG_VFLIP_EN |
> +  CSI_CH_CFG_HFLIP_EN | CSI_CH_CFG_FIELD_SEL_MASK |
> +  CSI_CH_CFG_INPUT_SEQ_MASK);
> +
> + val = get_csi_input_format(csi->config.code,
> csi->config.pixelformat);
> + cfg |= CSI_CH_CFG_INPUT_FMT(val);
> +
> + val = get_csi_output_format(csi->config.code,
> csi->config.field);
> + cfg |= CSI_CH_CFG_OUTPUT_FMT(val);
> +
> + val = get_csi_input_seq(csi->config.code,
> csi->config.pixelformat);
> + cfg |= CSI_CH_CFG_INPUT_SEQ(val);
> +
> + if (csi->config.field == V4L2_FIELD_TOP)
> + cfg |= CSI_CH_CFG_FIELD_SEL_FIELD0;
> + else if (csi->config.field == V4L2_FIELD_BOTTOM)
> + cfg |= CSI_CH_CFG_FIELD_SEL_FIELD1;
> + else
> + cfg |= CSI_CH_CFG_FIELD_SEL_BOTH;
> +
> + regmap_write(sdev->regmap, CSI_CH_CFG_REG, cfg);
> +}
> +
> +static void sun6i_csi_set_window(struct sun6i_csi_dev *sdev)
> +{
> + struct sun6i_csi_config *config = >csi.config;
> + u32 bytesperline_y;
> + u32 bytesperline_c;
> + int *planar_offset = sdev->planar_offset;
> +
> + regmap_write(sdev->regmap, CSI_CH_HSIZE_REG,
> +  CSI_CH_HSIZE_HOR_LEN(config->width) |
> +  CSI_CH_HSIZE_HOR_START(0));
> + regmap_write(sdev->regmap, CSI_CH_VSIZE_REG,
> +  CSI_CH_VSIZE_VER_LEN(config->height) |
> +  CSI_CH_VSIZE_VER_START(0));

In my case, the HOR_LEN and VER_LEN were not correctly configured.

They were configured to width and height (640 * 480) but as I was
using a YUYV format, the pixel's size is 2 bytes so the
horizontal/vertical lines' lengths were divided by 2.

Currently, I fixed that by getting the number of bytes per pixel with
"v4l2_pixformat_get_bpp()":

+   int bytes_pixel;
+
+   bytes_pixel = v4l2_pixformat_get_bpp(config->pixelformat) / 8;
 
regmap_write(sdev->regmap, CSI_CH_HSIZE_REG,
-CSI_CH_HSIZE_HOR_LEN(config->width) |
+CSI_CH_HSIZE_HOR_LEN(config->width * bytes_pixel) |
 CSI_CH_HSIZE_HOR_START(0));
regmap_write(sdev->regmap, CSI_CH_VSIZE_REG,
-CSI_CH_VSIZE_VER_LEN(config->height) |
+CSI_CH_VSIZE_VER_LEN(config->height * bytes_pixel)
  | CSI_CH_VSIZE_VER_START(0));

Re: [PATCH v2 1/3] media: V3s: Add support for Allwinner CSI.

[Yong]

Hi Maxime,

On Fri, 25 Aug 2017 15:41:14 +0200
Maxime Ripard  wrote:

> Hi Yong,
> 
> On Wed, Aug 23, 2017 at 10:32:16AM +0800, Yong wrote:
> > > > > > +static int sun6i_graph_notify_complete(struct v4l2_async_notifier 
> > > > > > *notifier)
> > > > > > +{
> > > > > > +   struct sun6i_csi *csi =
> > > > > > +   container_of(notifier, struct sun6i_csi, 
> > > > > > notifier);
> > > > > > +   struct sun6i_graph_entity *entity;
> > > > > > +   int ret;
> > > > > > +
> > > > > > +   dev_dbg(csi->dev, "notify complete, all subdevs registered\n");
> > > > > > +
> > > > > > +   /* Create links for every entity. */
> > > > > > +   list_for_each_entry(entity, >entities, list) {
> > > > > > +   ret = sun6i_graph_build_one(csi, entity);
> > > > > > +   if (ret < 0)
> > > > > > +   return ret;
> > > > > > +   }
> > > > > > +
> > > > > > +   /* Create links for video node. */
> > > > > > +   ret = sun6i_graph_build_video(csi);
> > > > > > +   if (ret < 0)
> > > > > > +   return ret;
> > > > > 
> > > > > Can you elaborate a bit on the difference between a node parsed with
> > > > > _graph_build_one and _graph_build_video? Can't you just store the
> > > > > remote sensor when you build the notifier, and reuse it here?
> > > > 
> > > > There maybe many usercases:
> > > > 1. CSI->Sensor.
> > > > 2. CSI->MIPI->Sensor.
> > > > 3. CSI->FPGA->Sensor1
> > > > ->Sensor2.
> > > > FPGA maybe some other video processor. FPGA, MIPI, Sensor can be
> > > > registered as v4l2 subdevs. We do not care about the driver code
> > > > of them. But they should be linked together here.
> > > > 
> > > > So, the _graph_build_one is used to link CSI port and subdevs. 
> > > > _graph_build_video is used to link CSI port and video node.
> > > 
> > > So the graph_build_one is for the two first cases, and the
> > > _build_video for the latter case?
> > 
> > No. 
> > The _graph_build_one is used to link the subdevs found in the device 
> > tree. _build_video is used to link the closest subdev to video node.
> > Video node is created in the driver, so the method to get it's pad is
> > diffrent to the subdevs.
> 
> Sorry for being slow here, I'm still not sure I get it.
> 
> In summary, both the sun6i_graph_build_one and sun6i_graph_build_video
> will iterate over each endpoint, will retrieve the remote entity, and
> will create the media link between the CSI pad and the remote pad.
> 
> As far as I can see, there's basically two things that
> sun6i_graph_build_one does that sun6i_graph_build_video doesn't:
>   - It skips all the links that would connect to one of the CSI sinks
>   - It skips all the links that would connect to a remote node that is
> equal to the CSI node.
> 
> I assume the latter is because you want to avoid going in an infinite
> loop when you would follow one of the CSI endpoint (going to the
> sensor), and then follow back the same link in the opposite
> direction. Right?

Not exactly. But any way, some code is true redundant here. I will 
make some improve.

> 
> I'm confused about the first one though. All the pads you create in
> your driver are sink pads, so wouldn't that skip all the pads of the
> CSI nodes?
> 
> Also, why do you iterate on all the CSI endpoints, when there's only
> of them? You want to anticipate the future binding for devices with
> multiple channels?
> 
> > > 
> > > If so, you should take a look at the last iteration of the
> > > subnotifiers rework by Nikas Söderlund (v4l2-async: add subnotifier
> > > registration for subdevices).
> > > 
> > > It allows subdevs to register notifiers, and you don't have to build
> > > the graph from the video device, each device and subdev can only care
> > > about what's next in the pipeline, but not really what's behind it.
> > > 
> > > That would mean in your case that you can only deal with your single
> > > CSI pad, and whatever subdev driver will use it care about its own.
> > 
> > Do you mean the subdevs create pad link in the notifier registered by
> > themself ?
> 
> Yes.
> 
> Thanks!
> Maxime
> 
> -- 
> Maxime Ripard, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com


Thanks,
Yong

Re: [PATCH v2 1/3] media: V3s: Add support for Allwinner CSI.

[Maxime Ripard]

Hi Yong,

On Wed, Aug 23, 2017 at 10:32:16AM +0800, Yong wrote:
> > > > > +static int sun6i_graph_notify_complete(struct v4l2_async_notifier 
> > > > > *notifier)
> > > > > +{
> > > > > + struct sun6i_csi *csi =
> > > > > + container_of(notifier, struct sun6i_csi, 
> > > > > notifier);
> > > > > + struct sun6i_graph_entity *entity;
> > > > > + int ret;
> > > > > +
> > > > > + dev_dbg(csi->dev, "notify complete, all subdevs registered\n");
> > > > > +
> > > > > + /* Create links for every entity. */
> > > > > + list_for_each_entry(entity, >entities, list) {
> > > > > + ret = sun6i_graph_build_one(csi, entity);
> > > > > + if (ret < 0)
> > > > > + return ret;
> > > > > + }
> > > > > +
> > > > > + /* Create links for video node. */
> > > > > + ret = sun6i_graph_build_video(csi);
> > > > > + if (ret < 0)
> > > > > + return ret;
> > > > 
> > > > Can you elaborate a bit on the difference between a node parsed with
> > > > _graph_build_one and _graph_build_video? Can't you just store the
> > > > remote sensor when you build the notifier, and reuse it here?
> > > 
> > > There maybe many usercases:
> > > 1. CSI->Sensor.
> > > 2. CSI->MIPI->Sensor.
> > > 3. CSI->FPGA->Sensor1
> > > ->Sensor2.
> > > FPGA maybe some other video processor. FPGA, MIPI, Sensor can be
> > > registered as v4l2 subdevs. We do not care about the driver code
> > > of them. But they should be linked together here.
> > > 
> > > So, the _graph_build_one is used to link CSI port and subdevs. 
> > > _graph_build_video is used to link CSI port and video node.
> > 
> > So the graph_build_one is for the two first cases, and the
> > _build_video for the latter case?
> 
> No. 
> The _graph_build_one is used to link the subdevs found in the device 
> tree. _build_video is used to link the closest subdev to video node.
> Video node is created in the driver, so the method to get it's pad is
> diffrent to the subdevs.

Sorry for being slow here, I'm still not sure I get it.

In summary, both the sun6i_graph_build_one and sun6i_graph_build_video
will iterate over each endpoint, will retrieve the remote entity, and
will create the media link between the CSI pad and the remote pad.

As far as I can see, there's basically two things that
sun6i_graph_build_one does that sun6i_graph_build_video doesn't:
  - It skips all the links that would connect to one of the CSI sinks
  - It skips all the links that would connect to a remote node that is
equal to the CSI node.

I assume the latter is because you want to avoid going in an infinite
loop when you would follow one of the CSI endpoint (going to the
sensor), and then follow back the same link in the opposite
direction. Right?

I'm confused about the first one though. All the pads you create in
your driver are sink pads, so wouldn't that skip all the pads of the
CSI nodes?

Also, why do you iterate on all the CSI endpoints, when there's only
of them? You want to anticipate the future binding for devices with
multiple channels?

> > 
> > If so, you should take a look at the last iteration of the
> > subnotifiers rework by Nikas Söderlund (v4l2-async: add subnotifier
> > registration for subdevices).
> > 
> > It allows subdevs to register notifiers, and you don't have to build
> > the graph from the video device, each device and subdev can only care
> > about what's next in the pipeline, but not really what's behind it.
> > 
> > That would mean in your case that you can only deal with your single
> > CSI pad, and whatever subdev driver will use it care about its own.
> 
> Do you mean the subdevs create pad link in the notifier registered by
> themself ?

Yes.

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com


signature.asc
Description: PGP signature

Re: [PATCH v2 1/3] media: V3s: Add support for Allwinner CSI.

[Yong]

On Wed, 23 Aug 2017 21:24:13 +0200
Maxime Ripard  wrote:

> On Wed, Aug 23, 2017 at 10:41:18AM +0800, Yong wrote:
> > > > > > +static irqreturn_t sun6i_csi_isr(int irq, void *dev_id)
> > > > > > +{
> > > > > > +   struct sun6i_csi_dev *sdev = (struct sun6i_csi_dev *)dev_id;
> > > > > > +   struct regmap *regmap = sdev->regmap;
> > > > > > +   u32 status;
> > > > > > +
> > > > > > +   regmap_read(regmap, CSI_CH_INT_STA_REG, );
> > > > > > +
> > > > > > +   if ((status & CSI_CH_INT_STA_FIFO0_OF_PD) ||
> > > > > > +   (status & CSI_CH_INT_STA_FIFO1_OF_PD) ||
> > > > > > +   (status & CSI_CH_INT_STA_FIFO2_OF_PD) ||
> > > > > > +   (status & CSI_CH_INT_STA_HB_OF_PD)) {
> > > > > > +   regmap_write(regmap, CSI_CH_INT_STA_REG, status);
> > > > > > +   regmap_update_bits(regmap, CSI_EN_REG, CSI_EN_CSI_EN, 
> > > > > > 0);
> > > > > > +   regmap_update_bits(regmap, CSI_EN_REG, CSI_EN_CSI_EN,
> > > > > > +  CSI_EN_CSI_EN);
> > > > > 
> > > > > You need to enable / disable it at every frame? How do you deal with
> > > > > double buffering? (or did you choose to ignore it for now?)
> > > > 
> > > > These *_OF_PD status bits indicate an overflow error condition.
> > > 
> > > Shouldn't we return an error code then? The names of these flags could
> > > be better too.
> > 
> > Then, where and how to deal with the error coce.
> 
> If you want to deal with FIFO overflow, I'm not sure you have anything
> to do. It means, you've been to slow to queue buffers, so I guess
> stopping the pipeline until more buffers are queued would make
> sense. And we should probably increase the sequence number while doing
> so to notify the userspace that some frames were lost.

If there is no queued buffers, the CSI must has been already stoped by
sun6i_video_frame_done. So, the FIFO overflow may only occur on some 
unpredictable conditions or something I don't know.

For sequence number, I can't actually get the number of the lost frames.

Maybe I misunderstood you. Did you mean use IRQ_RETVAL(error) instead
of IRQ_HANDLED?

> 
> Maxime
> 
> -- 
> Maxime Ripard, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com


Thanks,
Yong

Re: [PATCH v2 1/3] media: V3s: Add support for Allwinner CSI.

[Maxime Ripard]

On Wed, Aug 23, 2017 at 10:41:18AM +0800, Yong wrote:
> > > > > +static irqreturn_t sun6i_csi_isr(int irq, void *dev_id)
> > > > > +{
> > > > > + struct sun6i_csi_dev *sdev = (struct sun6i_csi_dev *)dev_id;
> > > > > + struct regmap *regmap = sdev->regmap;
> > > > > + u32 status;
> > > > > +
> > > > > + regmap_read(regmap, CSI_CH_INT_STA_REG, );
> > > > > +
> > > > > + if ((status & CSI_CH_INT_STA_FIFO0_OF_PD) ||
> > > > > + (status & CSI_CH_INT_STA_FIFO1_OF_PD) ||
> > > > > + (status & CSI_CH_INT_STA_FIFO2_OF_PD) ||
> > > > > + (status & CSI_CH_INT_STA_HB_OF_PD)) {
> > > > > + regmap_write(regmap, CSI_CH_INT_STA_REG, status);
> > > > > + regmap_update_bits(regmap, CSI_EN_REG, CSI_EN_CSI_EN, 
> > > > > 0);
> > > > > + regmap_update_bits(regmap, CSI_EN_REG, CSI_EN_CSI_EN,
> > > > > +CSI_EN_CSI_EN);
> > > > 
> > > > You need to enable / disable it at every frame? How do you deal with
> > > > double buffering? (or did you choose to ignore it for now?)
> > > 
> > > These *_OF_PD status bits indicate an overflow error condition.
> > 
> > Shouldn't we return an error code then? The names of these flags could
> > be better too.
> 
> Then, where and how to deal with the error coce.

If you want to deal with FIFO overflow, I'm not sure you have anything
to do. It means, you've been to slow to queue buffers, so I guess
stopping the pipeline until more buffers are queued would make
sense. And we should probably increase the sequence number while doing
so to notify the userspace that some frames were lost.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com


signature.asc
Description: PGP signature

Re: [PATCH v2 1/3] media: V3s: Add support for Allwinner CSI.

[icenowy]

在 2017-08-23 15:43，Laurent Pinchart 写道：

Hi Hans,

On Wednesday, 23 August 2017 09:52:00 EEST Hans Verkuil wrote:

On 08/22/2017 10:17 PM, Maxime Ripard wrote:
> On Tue, Aug 22, 2017 at 08:43:35AM +0200, Hans Verkuil wrote:
> +static int sun6i_video_link_setup(struct media_entity *entity,
> +const struct media_pad *local,
> +const struct media_pad *remote, u32 flags)
> +{
> +  struct video_device *vdev = media_entity_to_video_device(entity);
> +  struct sun6i_video *video = video_get_drvdata(vdev);
> +
> +  if (WARN_ON(video == NULL))
> +  return 0;
> +
> +  return sun6i_video_formats_init(video);

 Why is this called here? Why not in video_init()?
>>>
>>> sun6i_video_init is in the driver probe context.
>>> sun6i_video_formats_init use media_entity_remote_pad and
>>> media_entity_to_v4l2_subdev to find the subdevs.
>>> The media_entity_remote_pad can't work before all the media pad linked.
>>
>> A video_init is typically called from the notify_complete callback.
>> Actually, that's where the video_register_device should be created as
>> well. When you create it in probe() there is possibly no sensor yet, so
>> it would be a dead video node (or worse, crash when used).
>>
>> There are still a lot of platform drivers that create the video node in
>> the probe, but it's not the right place if you rely on the async loading
>> of subdevs.
>
> That's not really related, but I'm not really sure it's a good way to
> operate. This essentially means that you might wait forever for a
> component in your pipeline to be probed, without any chance of it
> happening (not compiled, compiled as a module and not loaded, hardware
> defect preventing the driver from probing properly, etc), even though
> that component might not be essential.

We're talking straightforward video pipelines here. I.e. a source, 
some

processing units and a DMA engine at the end.


As a first step possibly, but many SoCs have ISPs that are not 
supported by

the initial camera driver version.


I think here it's also the situation.

Allwinner SoCs beyond sun6i (which is this driver targeting at) has an 
ISP
internally called "HawkView ISP", and there's no document of it (even 
the

source code is only released in recent several months).



There is no point in creating a video node if the pipeline is not 
complete

since you need the full pipeline.

I've had bad experiences in the past where video nodes were created 
too
soon and part of the internal state was still incomplete, causing at 
best

weird behavior and at worst crashes.


Drivers obviously need to be fixed if they are buggy in that regard. 
Such race
conditions are definitely something I keep an eye on when reviewing 
code.



More complex devices are a whole different ballgame.

> This is how DRM operates, and you sometimes end up in some very dumb
> situations where you wait for say, the DSI controller to probe, while
> you only care about HDMI in your system.
>
> But this seems to be on of the hot topic these days, so we might
> discuss it further in some other thread :)

Re: [PATCH v2 1/3] media: V3s: Add support for Allwinner CSI.

[Laurent Pinchart]

Hi Hans,

On Wednesday, 23 August 2017 09:52:00 EEST Hans Verkuil wrote:
> On 08/22/2017 10:17 PM, Maxime Ripard wrote:
> > On Tue, Aug 22, 2017 at 08:43:35AM +0200, Hans Verkuil wrote:
> > +static int sun6i_video_link_setup(struct media_entity *entity,
> > + const struct media_pad *local,
> > + const struct media_pad *remote, u32 
> > flags)
> > +{
> > +   struct video_device *vdev = 
> > media_entity_to_video_device(entity);
> > +   struct sun6i_video *video = video_get_drvdata(vdev);
> > +
> > +   if (WARN_ON(video == NULL))
> > +   return 0;
> > +
> > +   return sun6i_video_formats_init(video);
>  
>  Why is this called here? Why not in video_init()?
> >>> 
> >>> sun6i_video_init is in the driver probe context. 
> >>> sun6i_video_formats_init use media_entity_remote_pad and
> >>> media_entity_to_v4l2_subdev to find the subdevs.
> >>> The media_entity_remote_pad can't work before all the media pad linked.
> >> 
> >> A video_init is typically called from the notify_complete callback.
> >> Actually, that's where the video_register_device should be created as
> >> well. When you create it in probe() there is possibly no sensor yet, so
> >> it would be a dead video node (or worse, crash when used).
> >> 
> >> There are still a lot of platform drivers that create the video node in
> >> the probe, but it's not the right place if you rely on the async loading
> >> of subdevs.
> > 
> > That's not really related, but I'm not really sure it's a good way to
> > operate. This essentially means that you might wait forever for a
> > component in your pipeline to be probed, without any chance of it
> > happening (not compiled, compiled as a module and not loaded, hardware
> > defect preventing the driver from probing properly, etc), even though
> > that component might not be essential.
> 
> We're talking straightforward video pipelines here. I.e. a source, some
> processing units and a DMA engine at the end.

As a first step possibly, but many SoCs have ISPs that are not supported by 
the initial camera driver version.

> There is no point in creating a video node if the pipeline is not complete
> since you need the full pipeline.
> 
> I've had bad experiences in the past where video nodes were created too
> soon and part of the internal state was still incomplete, causing at best
> weird behavior and at worst crashes.

Drivers obviously need to be fixed if they are buggy in that regard. Such race 
conditions are definitely something I keep an eye on when reviewing code.

> More complex devices are a whole different ballgame.
> 
> > This is how DRM operates, and you sometimes end up in some very dumb
> > situations where you wait for say, the DSI controller to probe, while
> > you only care about HDMI in your system.
> > 
> > But this seems to be on of the hot topic these days, so we might
> > discuss it further in some other thread :)

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 1/3] media: V3s: Add support for Allwinner CSI.

[Hans Verkuil]

On 08/22/2017 10:17 PM, Maxime Ripard wrote:
> On Tue, Aug 22, 2017 at 08:43:35AM +0200, Hans Verkuil wrote:
> +static int sun6i_video_link_setup(struct media_entity *entity,
> +   const struct media_pad *local,
> +   const struct media_pad *remote, u32 flags)
> +{
> + struct video_device *vdev = media_entity_to_video_device(entity);
> + struct sun6i_video *video = video_get_drvdata(vdev);
> +
> + if (WARN_ON(video == NULL))
> + return 0;
> +
> + return sun6i_video_formats_init(video);

 Why is this called here? Why not in video_init()?
>>>
>>> sun6i_video_init is in the driver probe context. sun6i_video_formats_init
>>> use media_entity_remote_pad and media_entity_to_v4l2_subdev to find the
>>> subdevs.
>>> The media_entity_remote_pad can't work before all the media pad linked.
>>
>> A video_init is typically called from the notify_complete callback.
>> Actually, that's where the video_register_device should be created as well.
>> When you create it in probe() there is possibly no sensor yet, so it would
>> be a dead video node (or worse, crash when used).
>>
>> There are still a lot of platform drivers that create the video node in the
>> probe, but it's not the right place if you rely on the async loading of
>> subdevs.
> 
> That's not really related, but I'm not really sure it's a good way to
> operate. This essentially means that you might wait forever for a
> component in your pipeline to be probed, without any chance of it
> happening (not compiled, compiled as a module and not loaded, hardware
> defect preventing the driver from probing properly, etc), even though
> that component might not be essential.

We're talking straightforward video pipelines here. I.e. a source, some
processing units and a DMA engine at the end. There is no point in
creating a video node if the pipeline is not complete since you need
the full pipeline.

I've had bad experiences in the past where video nodes were created too
soon and part of the internal state was still incomplete, causing at best
weird behavior and at worst crashes.

More complex devices are a whole different ballgame.

Regards,

Hans

> This is how DRM operates, and you sometimes end up in some very dumb
> situations where you wait for say, the DSI controller to probe, while
> you only care about HDMI in your system.
> 
> But this seems to be on of the hot topic these days, so we might
> discuss it further in some other thread :)
> 
> Maxime
> 

Re: [PATCH v2 1/3] media: V3s: Add support for Allwinner CSI.

[Yong]

On Mon, 21 Aug 2017 22:21:45 +0200
Maxime Ripard  wrote:

> Hi Baruch,
> 
> On Sun, Jul 30, 2017 at 09:08:01AM +0300, Baruch Siach wrote:
> > On Fri, Jul 28, 2017 at 06:02:33PM +0200, Maxime Ripard wrote:
> > > Hi, 
> > > 
> > > Thanks for the second iteration!
> > > 
> > > On Thu, Jul 27, 2017 at 01:01:35PM +0800, Yong Deng wrote:
> > > > Allwinner V3s SoC have two CSI module. CSI0 is used for MIPI interface
> > > > and CSI1 is used for parallel interface. This is not documented in
> > > > datasheet but by testing and guess.
> > > > 
> > > > This patch implement a v4l2 framework driver for it.
> > > > 
> > > > Currently, the driver only support the parallel interface. MIPI-CSI2,
> > > > ISP's support are not included in this patch.
> > > > 
> > > > Signed-off-by: Yong Deng 
> > 
> > [...]
> > 
> > > > +#ifdef DEBUG
> > > > +static void sun6i_csi_dump_regs(struct sun6i_csi_dev *sdev)
> > > > +{
> > > > +   struct regmap *regmap = sdev->regmap;
> > > > +   u32 val;
> > > > +
> > > > +   regmap_read(regmap, CSI_EN_REG, );
> > > > +   printk("CSI_EN_REG=0x%x\n", val);
> > > > +   regmap_read(regmap, CSI_IF_CFG_REG, );
> > > > +   printk("CSI_IF_CFG_REG=0x%x\n", val);
> > > > +   regmap_read(regmap, CSI_CAP_REG, );
> > > > +   printk("CSI_CAP_REG=0x%x\n",val);
> > > > +   regmap_read(regmap, CSI_SYNC_CNT_REG, );
> > > > +   printk("CSI_SYNC_CNT_REG=0x%x\n",   val);
> > > > +   regmap_read(regmap, CSI_FIFO_THRS_REG, );
> > > > +   printk("CSI_FIFO_THRS_REG=0x%x\n",  val);
> > > > +   regmap_read(regmap, CSI_PTN_LEN_REG, );
> > > > +   printk("CSI_PTN_LEN_REG=0x%x\n",val);
> > > > +   regmap_read(regmap, CSI_PTN_ADDR_REG, );
> > > > +   printk("CSI_PTN_ADDR_REG=0x%x\n",   val);
> > > > +   regmap_read(regmap, CSI_VER_REG, );
> > > > +   printk("CSI_VER_REG=0x%x\n",val);
> > > > +   regmap_read(regmap, CSI_CH_CFG_REG, );
> > > > +   printk("CSI_CH_CFG_REG=0x%x\n", val);
> > > > +   regmap_read(regmap, CSI_CH_SCALE_REG, );
> > > > +   printk("CSI_CH_SCALE_REG=0x%x\n",   val);
> > > > +   regmap_read(regmap, CSI_CH_F0_BUFA_REG, );
> > > > +   printk("CSI_CH_F0_BUFA_REG=0x%x\n", val);
> > > > +   regmap_read(regmap, CSI_CH_F1_BUFA_REG, );
> > > > +   printk("CSI_CH_F1_BUFA_REG=0x%x\n", val);
> > > > +   regmap_read(regmap, CSI_CH_F2_BUFA_REG, );
> > > > +   printk("CSI_CH_F2_BUFA_REG=0x%x\n", val);
> > > > +   regmap_read(regmap, CSI_CH_STA_REG, );
> > > > +   printk("CSI_CH_STA_REG=0x%x\n", val);
> > > > +   regmap_read(regmap, CSI_CH_INT_EN_REG, );
> > > > +   printk("CSI_CH_INT_EN_REG=0x%x\n",  val);
> > > > +   regmap_read(regmap, CSI_CH_INT_STA_REG, );
> > > > +   printk("CSI_CH_INT_STA_REG=0x%x\n", val);
> > > > +   regmap_read(regmap, CSI_CH_FLD1_VSIZE_REG, );
> > > > +   printk("CSI_CH_FLD1_VSIZE_REG=0x%x\n",  val);
> > > > +   regmap_read(regmap, CSI_CH_HSIZE_REG, );
> > > > +   printk("CSI_CH_HSIZE_REG=0x%x\n",   val);
> > > > +   regmap_read(regmap, CSI_CH_VSIZE_REG, );
> > > > +   printk("CSI_CH_VSIZE_REG=0x%x\n",   val);
> > > > +   regmap_read(regmap, CSI_CH_BUF_LEN_REG, );
> > > > +   printk("CSI_CH_BUF_LEN_REG=0x%x\n", val);
> > > > +   regmap_read(regmap, CSI_CH_FLIP_SIZE_REG, );
> > > > +   printk("CSI_CH_FLIP_SIZE_REG=0x%x\n",   val);
> > > > +   regmap_read(regmap, CSI_CH_FRM_CLK_CNT_REG, );
> > > > +   printk("CSI_CH_FRM_CLK_CNT_REG=0x%x\n", val);
> > > > +   regmap_read(regmap, CSI_CH_ACC_ITNL_CLK_CNT_REG, );
> > > > +   printk("CSI_CH_ACC_ITNL_CLK_CNT_REG=0x%x\n",val);
> > > > +   regmap_read(regmap, CSI_CH_FIFO_STAT_REG, );
> > > > +   printk("CSI_CH_FIFO_STAT_REG=0x%x\n",   val);
> > > > +   regmap_read(regmap, CSI_CH_PCLK_STAT_REG, );
> > > > +   printk("CSI_CH_PCLK_STAT_REG=0x%x\n",   val);
> > > > +}
> > > > +#endif
> > > 
> > > You can already dump a regmap through debugfs, that's redundant.
> > 
> > The advantage of in-code registers dump routine is the ability to
> > synchronize the snapshot with the driver code execution. This is
> > particularly important for the capture statistics registers. I have
> > found it useful here.
> 
> You also have the option to use the traces to do that, but if that's
> useful, this should be added to regmap itself. It can benefit others
> too.
> 
> > > > +static irqreturn_t sun6i_csi_isr(int irq, void *dev_id)
> > > > +{
> > > > +   struct sun6i_csi_dev *sdev = (struct sun6i_csi_dev *)dev_id;
> > > > +   struct regmap *regmap = sdev->regmap;
> > > > +   u32 status;
> > > > +
> > > > +   regmap_read(regmap, CSI_CH_INT_STA_REG, );
> > > > +
> > > > +   if ((status & CSI_CH_INT_STA_FIFO0_OF_PD) ||
> > > > +   (status & CSI_CH_INT_STA_FIFO1_OF_PD) ||

Re: [PATCH v2 1/3] media: V3s: Add support for Allwinner CSI.

[Yong]

On Tue, 22 Aug 2017 19:43:39 +0200
Maxime Ripard  wrote:

> Hi Yong,
> 
> On Mon, Jul 31, 2017 at 11:16:40AM +0800, Yong wrote:
> > > > @@ -143,6 +143,7 @@ source "drivers/media/platform/am437x/Kconfig"
> > > >  source "drivers/media/platform/xilinx/Kconfig"
> > > >  source "drivers/media/platform/rcar-vin/Kconfig"
> > > >  source "drivers/media/platform/atmel/Kconfig"
> > > > +source "drivers/media/platform/sun6i-csi/Kconfig"
> > > 
> > > We're going to have several different drivers in v4l eventually, so I
> > > guess it would make sense to move to a directory of our own.
> > 
> > Like this?
> > drivers/media/platform/sunxi/sun6i-csi
> 
> Yep.
> 
> > > > +static int sun6i_graph_notify_complete(struct v4l2_async_notifier 
> > > > *notifier)
> > > > +{
> > > > +   struct sun6i_csi *csi =
> > > > +   container_of(notifier, struct sun6i_csi, 
> > > > notifier);
> > > > +   struct sun6i_graph_entity *entity;
> > > > +   int ret;
> > > > +
> > > > +   dev_dbg(csi->dev, "notify complete, all subdevs registered\n");
> > > > +
> > > > +   /* Create links for every entity. */
> > > > +   list_for_each_entry(entity, >entities, list) {
> > > > +   ret = sun6i_graph_build_one(csi, entity);
> > > > +   if (ret < 0)
> > > > +   return ret;
> > > > +   }
> > > > +
> > > > +   /* Create links for video node. */
> > > > +   ret = sun6i_graph_build_video(csi);
> > > > +   if (ret < 0)
> > > > +   return ret;
> > > 
> > > Can you elaborate a bit on the difference between a node parsed with
> > > _graph_build_one and _graph_build_video? Can't you just store the
> > > remote sensor when you build the notifier, and reuse it here?
> > 
> > There maybe many usercases:
> > 1. CSI->Sensor.
> > 2. CSI->MIPI->Sensor.
> > 3. CSI->FPGA->Sensor1
> > ->Sensor2.
> > FPGA maybe some other video processor. FPGA, MIPI, Sensor can be
> > registered as v4l2 subdevs. We do not care about the driver code
> > of them. But they should be linked together here.
> > 
> > So, the _graph_build_one is used to link CSI port and subdevs. 
> > _graph_build_video is used to link CSI port and video node.
> 
> So the graph_build_one is for the two first cases, and the
> _build_video for the latter case?

No. 
The _graph_build_one is used to link the subdevs found in the device 
tree. _build_video is used to link the closest subdev to video node.
Video node is created in the driver, so the method to get it's pad is
diffrent to the subdevs.

> 
> If so, you should take a look at the last iteration of the
> subnotifiers rework by Nikas Söderlund (v4l2-async: add subnotifier
> registration for subdevices).
> 
> It allows subdevs to register notifiers, and you don't have to build
> the graph from the video device, each device and subdev can only care
> about what's next in the pipeline, but not really what's behind it.
> 
> That would mean in your case that you can only deal with your single
> CSI pad, and whatever subdev driver will use it care about its own.

Do you mean the subdevs create pad link in the notifier registered by
themself ?
If so, _graph_build_one is needless. But how to make sure the pipeline
has linked correctly when operateing the pipeline. 
I will lookt at this in more detail.

> 
> > This part is also difficult to understand for me. The one CSI module
> > have only one DMA channel(single port). But thay can be linked to 
> > different physical port (Parallel or MIPI)(multiple ep) by IF select
> > register.
> > 
> > For now, the binding can have several ep, the driver will just pick
> > the first valid one.
> 
> Yeah, I'm not really sure how we could deal with that, but I guess we
> can do it later on.
> 
> > > 
> > > > +struct sun6i_csi_ops {
> > > > +   int (*get_supported_pixformats)(struct sun6i_csi *csi,
> > > > +   const u32 **pixformats);
> > > > +   bool (*is_format_support)(struct sun6i_csi *csi, u32 pixformat,
> > > > + u32 mbus_code);
> > > > +   int (*s_power)(struct sun6i_csi *csi, bool enable);
> > > > +   int (*update_config)(struct sun6i_csi *csi,
> > > > +struct sun6i_csi_config *config);
> > > > +   int (*update_buf_addr)(struct sun6i_csi *csi, dma_addr_t addr);
> > > > +   int (*s_stream)(struct sun6i_csi *csi, bool enable);
> > > > +};
> > > 
> > > Didn't we agreed on removing those in the first iteration? It's not
> > > really clear at this point whether they will be needed at all. Make
> > > something simple first, without those ops. When we'll support other
> > > SoCs we'll have a better chance at seeing what and how we should deal
> > > with potential quirks.
> > 
> > OK. But without ops, it is inappropriate to sun6i_csi and sun6i_csi.
> > Maybe I should merge the two files.
> 
> I'm not sure what you meant here, but if you think that's 

Re: [PATCH v2 1/3] media: V3s: Add support for Allwinner CSI.

[Laurent Pinchart]

Hello,

On Tuesday, 22 August 2017 23:17:31 EEST Maxime Ripard wrote:
> On Tue, Aug 22, 2017 at 08:43:35AM +0200, Hans Verkuil wrote:
>  +static int sun6i_video_link_setup(struct media_entity *entity,
>  +  const struct media_pad *local,
>  +  const struct media_pad *remote, u32 
>  flags)
>  +{
>  +struct video_device *vdev = 
>  media_entity_to_video_device(entity);
>  +struct sun6i_video *video = video_get_drvdata(vdev);
>  +
>  +if (WARN_ON(video == NULL))
>  +return 0;
>  +
>  +return sun6i_video_formats_init(video);
> >>> 
> >>> Why is this called here? Why not in video_init()?
> >> 
> >> sun6i_video_init is in the driver probe context.
> >> sun6i_video_formats_init use media_entity_remote_pad and
> >> media_entity_to_v4l2_subdev to find the subdevs.
> >> The media_entity_remote_pad can't work before all the media pad linked.
> > 
> > A video_init is typically called from the notify_complete callback.
> > Actually, that's where the video_register_device should be created as
> > well. When you create it in probe() there is possibly no sensor yet, so it
> > would be a dead video node (or worse, crash when used).
> > 
> > There are still a lot of platform drivers that create the video node in
> > the probe, but it's not the right place if you rely on the async loading
> > of subdevs.
> 
> That's not really related, but I'm not really sure it's a good way to
> operate. This essentially means that you might wait forever for a
> component in your pipeline to be probed, without any chance of it
> happening (not compiled, compiled as a module and not loaded, hardware
> defect preventing the driver from probing properly, etc), even though
> that component might not be essential.

I agree with Maxime here, we should build the media device incrementally, and 
offer userspace access early on without waiting for all pieces to be 
available. If properly implemented (there should definitely be so crash) I 
don't see any drawback to that approach.

> This is how DRM operates, and you sometimes end up in some very dumb
> situations where you wait for say, the DSI controller to probe, while
> you only care about HDMI in your system.
> 
> But this seems to be on of the hot topic these days, so we might
> discuss it further in some other thread :)

Or here :-)

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 1/3] media: V3s: Add support for Allwinner CSI.

[Maxime Ripard]

On Tue, Aug 22, 2017 at 08:43:35AM +0200, Hans Verkuil wrote:
> >>> +static int sun6i_video_link_setup(struct media_entity *entity,
> >>> +   const struct media_pad *local,
> >>> +   const struct media_pad *remote, u32 flags)
> >>> +{
> >>> + struct video_device *vdev = media_entity_to_video_device(entity);
> >>> + struct sun6i_video *video = video_get_drvdata(vdev);
> >>> +
> >>> + if (WARN_ON(video == NULL))
> >>> + return 0;
> >>> +
> >>> + return sun6i_video_formats_init(video);
> >>
> >> Why is this called here? Why not in video_init()?
> > 
> > sun6i_video_init is in the driver probe context. sun6i_video_formats_init
> > use media_entity_remote_pad and media_entity_to_v4l2_subdev to find the
> > subdevs.
> > The media_entity_remote_pad can't work before all the media pad linked.
> 
> A video_init is typically called from the notify_complete callback.
> Actually, that's where the video_register_device should be created as well.
> When you create it in probe() there is possibly no sensor yet, so it would
> be a dead video node (or worse, crash when used).
> 
> There are still a lot of platform drivers that create the video node in the
> probe, but it's not the right place if you rely on the async loading of
> subdevs.

That's not really related, but I'm not really sure it's a good way to
operate. This essentially means that you might wait forever for a
component in your pipeline to be probed, without any chance of it
happening (not compiled, compiled as a module and not loaded, hardware
defect preventing the driver from probing properly, etc), even though
that component might not be essential.

This is how DRM operates, and you sometimes end up in some very dumb
situations where you wait for say, the DSI controller to probe, while
you only care about HDMI in your system.

But this seems to be on of the hot topic these days, so we might
discuss it further in some other thread :)

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com


signature.asc
Description: PGP signature

Re: [PATCH v2 1/3] media: V3s: Add support for Allwinner CSI.

[Maxime Ripard]

Hi Yong,

On Mon, Jul 31, 2017 at 11:16:40AM +0800, Yong wrote:
> > > @@ -143,6 +143,7 @@ source "drivers/media/platform/am437x/Kconfig"
> > >  source "drivers/media/platform/xilinx/Kconfig"
> > >  source "drivers/media/platform/rcar-vin/Kconfig"
> > >  source "drivers/media/platform/atmel/Kconfig"
> > > +source "drivers/media/platform/sun6i-csi/Kconfig"
> > 
> > We're going to have several different drivers in v4l eventually, so I
> > guess it would make sense to move to a directory of our own.
> 
> Like this?
> drivers/media/platform/sunxi/sun6i-csi

Yep.

> > > +static int sun6i_graph_notify_complete(struct v4l2_async_notifier 
> > > *notifier)
> > > +{
> > > + struct sun6i_csi *csi =
> > > + container_of(notifier, struct sun6i_csi, notifier);
> > > + struct sun6i_graph_entity *entity;
> > > + int ret;
> > > +
> > > + dev_dbg(csi->dev, "notify complete, all subdevs registered\n");
> > > +
> > > + /* Create links for every entity. */
> > > + list_for_each_entry(entity, >entities, list) {
> > > + ret = sun6i_graph_build_one(csi, entity);
> > > + if (ret < 0)
> > > + return ret;
> > > + }
> > > +
> > > + /* Create links for video node. */
> > > + ret = sun6i_graph_build_video(csi);
> > > + if (ret < 0)
> > > + return ret;
> > 
> > Can you elaborate a bit on the difference between a node parsed with
> > _graph_build_one and _graph_build_video? Can't you just store the
> > remote sensor when you build the notifier, and reuse it here?
> 
> There maybe many usercases:
> 1. CSI->Sensor.
> 2. CSI->MIPI->Sensor.
> 3. CSI->FPGA->Sensor1
> ->Sensor2.
> FPGA maybe some other video processor. FPGA, MIPI, Sensor can be
> registered as v4l2 subdevs. We do not care about the driver code
> of them. But they should be linked together here.
> 
> So, the _graph_build_one is used to link CSI port and subdevs. 
> _graph_build_video is used to link CSI port and video node.

So the graph_build_one is for the two first cases, and the
_build_video for the latter case?

If so, you should take a look at the last iteration of the
subnotifiers rework by Nikas Söderlund (v4l2-async: add subnotifier
registration for subdevices).

It allows subdevs to register notifiers, and you don't have to build
the graph from the video device, each device and subdev can only care
about what's next in the pipeline, but not really what's behind it.

That would mean in your case that you can only deal with your single
CSI pad, and whatever subdev driver will use it care about its own.

> This part is also difficult to understand for me. The one CSI module
> have only one DMA channel(single port). But thay can be linked to 
> different physical port (Parallel or MIPI)(multiple ep) by IF select
> register.
> 
> For now, the binding can have several ep, the driver will just pick
> the first valid one.

Yeah, I'm not really sure how we could deal with that, but I guess we
can do it later on.

> > 
> > > +struct sun6i_csi_ops {
> > > + int (*get_supported_pixformats)(struct sun6i_csi *csi,
> > > + const u32 **pixformats);
> > > + bool (*is_format_support)(struct sun6i_csi *csi, u32 pixformat,
> > > +   u32 mbus_code);
> > > + int (*s_power)(struct sun6i_csi *csi, bool enable);
> > > + int (*update_config)(struct sun6i_csi *csi,
> > > +  struct sun6i_csi_config *config);
> > > + int (*update_buf_addr)(struct sun6i_csi *csi, dma_addr_t addr);
> > > + int (*s_stream)(struct sun6i_csi *csi, bool enable);
> > > +};
> > 
> > Didn't we agreed on removing those in the first iteration? It's not
> > really clear at this point whether they will be needed at all. Make
> > something simple first, without those ops. When we'll support other
> > SoCs we'll have a better chance at seeing what and how we should deal
> > with potential quirks.
> 
> OK. But without ops, it is inappropriate to sun6i_csi and sun6i_csi.
> Maybe I should merge the two files.

I'm not sure what you meant here, but if you think that's appropriate,
please go ahead.

> > > + return IRQ_HANDLED;
> > > + }
> > > +
> > > + if (status & CSI_CH_INT_STA_FD_PD) {
> > > + sun6i_video_frame_done(>csi.video);
> > > + }
> > > +
> > > + regmap_write(regmap, CSI_CH_INT_STA_REG, status);
> > 
> > Isn't it redundant with the one you did in the condition a bit above?
> > 
> > You should also check that your device indeed generated an
> > interrupt. In the occurence of a spourious interrupt, your code will
> > return IRQ_HANDLED, which is the wrong thing to do.
> > 
> > I think you should reverse your logic a bit here to make this
> > easier. You should just check that your status flags are indeed set,
> > and if not just bail out and return IRQ_NONE.
> > 
> > And if they are, go on with treating your interrupt.
> 
> OK. I will add check for status flags.
> BTW, how can a spurious interrupt occurred?

Usually it's either through some interference, or some 

Re: [PATCH v2 1/3] media: V3s: Add support for Allwinner CSI.

[Yong]

On Tue, 22 Aug 2017 08:43:35 +0200
Hans Verkuil  wrote:

> On 08/22/2017 05:01 AM, Yong wrote:
> > Hi Hans,
> > 
> > On Mon, 21 Aug 2017 16:37:41 +0200
> > Hans Verkuil  wrote:
> > 
> >> Hi Yong,
> >>
> >> First two high-level comments before I start the review:
> >>
> >> 1) Can you provide the v4l2-compliance output? I can't merge this unless I
> >>see that it passes the compliance tests. Make sure you compile from the 
> >> git
> >>repo (https://git.linuxtv.org/v4l-utils.git/) so you are using the 
> >> latest
> >>version of the compliance test. Test with just 'v4l2-compliance' and 
> >> also
> >>with the -s option (to test streaming) and with the -f option (to test 
> >> all
> >>the various pixel formats).
> > 
> > OK. I will post with the next version.
> > 
> >>
> >> 2) I see you support interlaced formats. Did you actually test that? 
> >> Interlaced
> >>is tricky and you shouldn't add support for it unless you know it works 
> >> and
> >>that it passes the 'v4l2-compliance -s' test.
> > 
> > No. I do not have the condition to test the all formats for now. Maybe this 
> > can 
> > be done when ourself's device with V3s is ready in the future months.
> 
> Then just drop the interlaced support until you can test it.

OK. I will try to find a way to test it. If not, I will drop it.

> 
> > 
> >>
> >> On 07/27/2017 07:01 AM, Yong Deng wrote:
> >>> Allwinner V3s SoC have two CSI module. CSI0 is used for MIPI interface
> >>> and CSI1 is used for parallel interface. This is not documented in
> >>> datasheet but by testing and guess.
> >>>
> >>> This patch implement a v4l2 framework driver for it.
> >>>
> >>> Currently, the driver only support the parallel interface. MIPI-CSI2,
> >>> ISP's support are not included in this patch.
> >>>
> >>> Signed-off-by: Yong Deng 
> >>> ---
> >>>  drivers/media/platform/Kconfig   |   1 +
> >>>  drivers/media/platform/Makefile  |   2 +
> >>>  drivers/media/platform/sun6i-csi/Kconfig |   9 +
> >>>  drivers/media/platform/sun6i-csi/Makefile|   3 +
> >>>  drivers/media/platform/sun6i-csi/sun6i_csi.c | 545 +++
> >>>  drivers/media/platform/sun6i-csi/sun6i_csi.h | 203 ++
> >>>  drivers/media/platform/sun6i-csi/sun6i_csi_v3s.c | 827 
> >>> +++
> >>>  drivers/media/platform/sun6i-csi/sun6i_csi_v3s.h | 206 ++
> >>>  drivers/media/platform/sun6i-csi/sun6i_video.c   | 663 ++
> >>>  drivers/media/platform/sun6i-csi/sun6i_video.h   |  61 ++
> >>>  10 files changed, 2520 insertions(+)
> >>>  create mode 100644 drivers/media/platform/sun6i-csi/Kconfig
> >>>  create mode 100644 drivers/media/platform/sun6i-csi/Makefile
> >>>  create mode 100644 drivers/media/platform/sun6i-csi/sun6i_csi.c
> >>>  create mode 100644 drivers/media/platform/sun6i-csi/sun6i_csi.h
> >>>  create mode 100644 drivers/media/platform/sun6i-csi/sun6i_csi_v3s.c
> >>>  create mode 100644 drivers/media/platform/sun6i-csi/sun6i_csi_v3s.h
> >>>  create mode 100644 drivers/media/platform/sun6i-csi/sun6i_video.c
> >>>  create mode 100644 drivers/media/platform/sun6i-csi/sun6i_video.h
> >>>
> >>
> >> 
> >>
> >>> diff --git a/drivers/media/platform/sun6i-csi/sun6i_video.c 
> >>> b/drivers/media/platform/sun6i-csi/sun6i_video.c
> >>> new file mode 100644
> >>> index 000..0c5dbd2
> >>> --- /dev/null
> >>> +++ b/drivers/media/platform/sun6i-csi/sun6i_video.c
> >>> @@ -0,0 +1,663 @@
> >>> +/*
> >>> + * Copyright (c) 2017 Magewell Electronics Co., Ltd. (Nanjing).
> >>> + * All rights reserved.
> >>> + * Author: Yong Deng 
> >>> + *
> >>> + * This program is free software; you can redistribute it and/or modify
> >>> + * it under the terms of the GNU General Public License version 2 as
> >>> + * published by the Free Software Foundation.
> >>> + *
> >>> + * This program is distributed in the hope that it will be useful,
> >>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> >>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> >>> + * GNU General Public License for more details.
> >>> + */
> >>> +
> >>> +#include 
> >>> +
> >>> +#include 
> >>> +#include 
> >>> +#include 
> >>> +#include 
> >>> +#include 
> >>> +
> >>> +#include "sun6i_csi.h"
> >>> +#include "sun6i_video.h"
> >>> +
> >>> +struct sun6i_csi_buffer {
> >>> + struct vb2_v4l2_buffer  vb;
> >>> + struct list_headlist;
> >>> +
> >>> + dma_addr_t  dma_addr;
> >>> +};
> >>> +
> >>> +static struct sun6i_csi_format *
> >>> +find_format_by_fourcc(struct sun6i_video *video, unsigned int fourcc)
> >>> +{
> >>> + unsigned int num_formats = video->num_formats;
> >>> + struct sun6i_csi_format *fmt;
> >>> + unsigned int i;
> >>> +
> >>> + for (i = 0; i < num_formats; i++) {
> >>> + fmt = >formats[i];
> >>> + if (fmt->fourcc == fourcc)
> >>> + 

Re: [PATCH v2 1/3] media: V3s: Add support for Allwinner CSI.

[Hans Verkuil]

On 08/22/2017 05:01 AM, Yong wrote:
> Hi Hans,
> 
> On Mon, 21 Aug 2017 16:37:41 +0200
> Hans Verkuil  wrote:
> 
>> Hi Yong,
>>
>> First two high-level comments before I start the review:
>>
>> 1) Can you provide the v4l2-compliance output? I can't merge this unless I
>>see that it passes the compliance tests. Make sure you compile from the 
>> git
>>repo (https://git.linuxtv.org/v4l-utils.git/) so you are using the latest
>>version of the compliance test. Test with just 'v4l2-compliance' and also
>>with the -s option (to test streaming) and with the -f option (to test all
>>the various pixel formats).
> 
> OK. I will post with the next version.
> 
>>
>> 2) I see you support interlaced formats. Did you actually test that? 
>> Interlaced
>>is tricky and you shouldn't add support for it unless you know it works 
>> and
>>that it passes the 'v4l2-compliance -s' test.
> 
> No. I do not have the condition to test the all formats for now. Maybe this 
> can 
> be done when ourself's device with V3s is ready in the future months.

Then just drop the interlaced support until you can test it.

> 
>>
>> On 07/27/2017 07:01 AM, Yong Deng wrote:
>>> Allwinner V3s SoC have two CSI module. CSI0 is used for MIPI interface
>>> and CSI1 is used for parallel interface. This is not documented in
>>> datasheet but by testing and guess.
>>>
>>> This patch implement a v4l2 framework driver for it.
>>>
>>> Currently, the driver only support the parallel interface. MIPI-CSI2,
>>> ISP's support are not included in this patch.
>>>
>>> Signed-off-by: Yong Deng 
>>> ---
>>>  drivers/media/platform/Kconfig   |   1 +
>>>  drivers/media/platform/Makefile  |   2 +
>>>  drivers/media/platform/sun6i-csi/Kconfig |   9 +
>>>  drivers/media/platform/sun6i-csi/Makefile|   3 +
>>>  drivers/media/platform/sun6i-csi/sun6i_csi.c | 545 +++
>>>  drivers/media/platform/sun6i-csi/sun6i_csi.h | 203 ++
>>>  drivers/media/platform/sun6i-csi/sun6i_csi_v3s.c | 827 
>>> +++
>>>  drivers/media/platform/sun6i-csi/sun6i_csi_v3s.h | 206 ++
>>>  drivers/media/platform/sun6i-csi/sun6i_video.c   | 663 ++
>>>  drivers/media/platform/sun6i-csi/sun6i_video.h   |  61 ++
>>>  10 files changed, 2520 insertions(+)
>>>  create mode 100644 drivers/media/platform/sun6i-csi/Kconfig
>>>  create mode 100644 drivers/media/platform/sun6i-csi/Makefile
>>>  create mode 100644 drivers/media/platform/sun6i-csi/sun6i_csi.c
>>>  create mode 100644 drivers/media/platform/sun6i-csi/sun6i_csi.h
>>>  create mode 100644 drivers/media/platform/sun6i-csi/sun6i_csi_v3s.c
>>>  create mode 100644 drivers/media/platform/sun6i-csi/sun6i_csi_v3s.h
>>>  create mode 100644 drivers/media/platform/sun6i-csi/sun6i_video.c
>>>  create mode 100644 drivers/media/platform/sun6i-csi/sun6i_video.h
>>>
>>
>> 
>>
>>> diff --git a/drivers/media/platform/sun6i-csi/sun6i_video.c 
>>> b/drivers/media/platform/sun6i-csi/sun6i_video.c
>>> new file mode 100644
>>> index 000..0c5dbd2
>>> --- /dev/null
>>> +++ b/drivers/media/platform/sun6i-csi/sun6i_video.c
>>> @@ -0,0 +1,663 @@
>>> +/*
>>> + * Copyright (c) 2017 Magewell Electronics Co., Ltd. (Nanjing).
>>> + * All rights reserved.
>>> + * Author: Yong Deng 
>>> + *
>>> + * This program is free software; you can redistribute it and/or modify
>>> + * it under the terms of the GNU General Public License version 2 as
>>> + * published by the Free Software Foundation.
>>> + *
>>> + * This program is distributed in the hope that it will be useful,
>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>>> + * GNU General Public License for more details.
>>> + */
>>> +
>>> +#include 
>>> +
>>> +#include 
>>> +#include 
>>> +#include 
>>> +#include 
>>> +#include 
>>> +
>>> +#include "sun6i_csi.h"
>>> +#include "sun6i_video.h"
>>> +
>>> +struct sun6i_csi_buffer {
>>> +   struct vb2_v4l2_buffer  vb;
>>> +   struct list_headlist;
>>> +
>>> +   dma_addr_t  dma_addr;
>>> +};
>>> +
>>> +static struct sun6i_csi_format *
>>> +find_format_by_fourcc(struct sun6i_video *video, unsigned int fourcc)
>>> +{
>>> +   unsigned int num_formats = video->num_formats;
>>> +   struct sun6i_csi_format *fmt;
>>> +   unsigned int i;
>>> +
>>> +   for (i = 0; i < num_formats; i++) {
>>> +   fmt = >formats[i];
>>> +   if (fmt->fourcc == fourcc)
>>> +   return fmt;
>>> +   }
>>> +
>>> +   return NULL;
>>> +}
>>> +
>>> +static struct v4l2_subdev *
>>> +sun6i_video_remote_subdev(struct sun6i_video *video, u32 *pad)
>>> +{
>>> +   struct media_pad *remote;
>>> +
>>> +   remote = media_entity_remote_pad(>pad);
>>> +
>>> +   if (!remote || !is_media_entity_v4l2_subdev(remote->entity))
>>> +   return NULL;
>>> +
>>> +   if (pad)

Re: [PATCH v2 1/3] media: V3s: Add support for Allwinner CSI.

[Yong]

Hi Hans,

On Mon, 21 Aug 2017 16:37:41 +0200
Hans Verkuil  wrote:

> Hi Yong,
> 
> First two high-level comments before I start the review:
> 
> 1) Can you provide the v4l2-compliance output? I can't merge this unless I
>see that it passes the compliance tests. Make sure you compile from the git
>repo (https://git.linuxtv.org/v4l-utils.git/) so you are using the latest
>version of the compliance test. Test with just 'v4l2-compliance' and also
>with the -s option (to test streaming) and with the -f option (to test all
>the various pixel formats).

OK. I will post with the next version.

> 
> 2) I see you support interlaced formats. Did you actually test that? 
> Interlaced
>is tricky and you shouldn't add support for it unless you know it works and
>that it passes the 'v4l2-compliance -s' test.

No. I do not have the condition to test the all formats for now. Maybe this can 
be done when ourself's device with V3s is ready in the future months.

> 
> On 07/27/2017 07:01 AM, Yong Deng wrote:
> > Allwinner V3s SoC have two CSI module. CSI0 is used for MIPI interface
> > and CSI1 is used for parallel interface. This is not documented in
> > datasheet but by testing and guess.
> > 
> > This patch implement a v4l2 framework driver for it.
> > 
> > Currently, the driver only support the parallel interface. MIPI-CSI2,
> > ISP's support are not included in this patch.
> > 
> > Signed-off-by: Yong Deng 
> > ---
> >  drivers/media/platform/Kconfig   |   1 +
> >  drivers/media/platform/Makefile  |   2 +
> >  drivers/media/platform/sun6i-csi/Kconfig |   9 +
> >  drivers/media/platform/sun6i-csi/Makefile|   3 +
> >  drivers/media/platform/sun6i-csi/sun6i_csi.c | 545 +++
> >  drivers/media/platform/sun6i-csi/sun6i_csi.h | 203 ++
> >  drivers/media/platform/sun6i-csi/sun6i_csi_v3s.c | 827 
> > +++
> >  drivers/media/platform/sun6i-csi/sun6i_csi_v3s.h | 206 ++
> >  drivers/media/platform/sun6i-csi/sun6i_video.c   | 663 ++
> >  drivers/media/platform/sun6i-csi/sun6i_video.h   |  61 ++
> >  10 files changed, 2520 insertions(+)
> >  create mode 100644 drivers/media/platform/sun6i-csi/Kconfig
> >  create mode 100644 drivers/media/platform/sun6i-csi/Makefile
> >  create mode 100644 drivers/media/platform/sun6i-csi/sun6i_csi.c
> >  create mode 100644 drivers/media/platform/sun6i-csi/sun6i_csi.h
> >  create mode 100644 drivers/media/platform/sun6i-csi/sun6i_csi_v3s.c
> >  create mode 100644 drivers/media/platform/sun6i-csi/sun6i_csi_v3s.h
> >  create mode 100644 drivers/media/platform/sun6i-csi/sun6i_video.c
> >  create mode 100644 drivers/media/platform/sun6i-csi/sun6i_video.h
> > 
> 
> 
> 
> > diff --git a/drivers/media/platform/sun6i-csi/sun6i_video.c 
> > b/drivers/media/platform/sun6i-csi/sun6i_video.c
> > new file mode 100644
> > index 000..0c5dbd2
> > --- /dev/null
> > +++ b/drivers/media/platform/sun6i-csi/sun6i_video.c
> > @@ -0,0 +1,663 @@
> > +/*
> > + * Copyright (c) 2017 Magewell Electronics Co., Ltd. (Nanjing).
> > + * All rights reserved.
> > + * Author: Yong Deng 
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License version 2 as
> > + * published by the Free Software Foundation.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + * GNU General Public License for more details.
> > + */
> > +
> > +#include 
> > +
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +
> > +#include "sun6i_csi.h"
> > +#include "sun6i_video.h"
> > +
> > +struct sun6i_csi_buffer {
> > +   struct vb2_v4l2_buffer  vb;
> > +   struct list_headlist;
> > +
> > +   dma_addr_t  dma_addr;
> > +};
> > +
> > +static struct sun6i_csi_format *
> > +find_format_by_fourcc(struct sun6i_video *video, unsigned int fourcc)
> > +{
> > +   unsigned int num_formats = video->num_formats;
> > +   struct sun6i_csi_format *fmt;
> > +   unsigned int i;
> > +
> > +   for (i = 0; i < num_formats; i++) {
> > +   fmt = >formats[i];
> > +   if (fmt->fourcc == fourcc)
> > +   return fmt;
> > +   }
> > +
> > +   return NULL;
> > +}
> > +
> > +static struct v4l2_subdev *
> > +sun6i_video_remote_subdev(struct sun6i_video *video, u32 *pad)
> > +{
> > +   struct media_pad *remote;
> > +
> > +   remote = media_entity_remote_pad(>pad);
> > +
> > +   if (!remote || !is_media_entity_v4l2_subdev(remote->entity))
> > +   return NULL;
> > +
> > +   if (pad)
> > +   *pad = remote->index;
> > +
> > +   return media_entity_to_v4l2_subdev(remote->entity);
> > +}
> > +
> > +static int 

Re: [PATCH v2 1/3] media: V3s: Add support for Allwinner CSI.

[Maxime Ripard]

Hi Baruch,

On Sun, Jul 30, 2017 at 09:08:01AM +0300, Baruch Siach wrote:
> On Fri, Jul 28, 2017 at 06:02:33PM +0200, Maxime Ripard wrote:
> > Hi, 
> > 
> > Thanks for the second iteration!
> > 
> > On Thu, Jul 27, 2017 at 01:01:35PM +0800, Yong Deng wrote:
> > > Allwinner V3s SoC have two CSI module. CSI0 is used for MIPI interface
> > > and CSI1 is used for parallel interface. This is not documented in
> > > datasheet but by testing and guess.
> > > 
> > > This patch implement a v4l2 framework driver for it.
> > > 
> > > Currently, the driver only support the parallel interface. MIPI-CSI2,
> > > ISP's support are not included in this patch.
> > > 
> > > Signed-off-by: Yong Deng 
> 
> [...]
> 
> > > +#ifdef DEBUG
> > > +static void sun6i_csi_dump_regs(struct sun6i_csi_dev *sdev)
> > > +{
> > > + struct regmap *regmap = sdev->regmap;
> > > + u32 val;
> > > +
> > > + regmap_read(regmap, CSI_EN_REG, );
> > > + printk("CSI_EN_REG=0x%x\n", val);
> > > + regmap_read(regmap, CSI_IF_CFG_REG, );
> > > + printk("CSI_IF_CFG_REG=0x%x\n", val);
> > > + regmap_read(regmap, CSI_CAP_REG, );
> > > + printk("CSI_CAP_REG=0x%x\n",val);
> > > + regmap_read(regmap, CSI_SYNC_CNT_REG, );
> > > + printk("CSI_SYNC_CNT_REG=0x%x\n",   val);
> > > + regmap_read(regmap, CSI_FIFO_THRS_REG, );
> > > + printk("CSI_FIFO_THRS_REG=0x%x\n",  val);
> > > + regmap_read(regmap, CSI_PTN_LEN_REG, );
> > > + printk("CSI_PTN_LEN_REG=0x%x\n",val);
> > > + regmap_read(regmap, CSI_PTN_ADDR_REG, );
> > > + printk("CSI_PTN_ADDR_REG=0x%x\n",   val);
> > > + regmap_read(regmap, CSI_VER_REG, );
> > > + printk("CSI_VER_REG=0x%x\n",val);
> > > + regmap_read(regmap, CSI_CH_CFG_REG, );
> > > + printk("CSI_CH_CFG_REG=0x%x\n", val);
> > > + regmap_read(regmap, CSI_CH_SCALE_REG, );
> > > + printk("CSI_CH_SCALE_REG=0x%x\n",   val);
> > > + regmap_read(regmap, CSI_CH_F0_BUFA_REG, );
> > > + printk("CSI_CH_F0_BUFA_REG=0x%x\n", val);
> > > + regmap_read(regmap, CSI_CH_F1_BUFA_REG, );
> > > + printk("CSI_CH_F1_BUFA_REG=0x%x\n", val);
> > > + regmap_read(regmap, CSI_CH_F2_BUFA_REG, );
> > > + printk("CSI_CH_F2_BUFA_REG=0x%x\n", val);
> > > + regmap_read(regmap, CSI_CH_STA_REG, );
> > > + printk("CSI_CH_STA_REG=0x%x\n", val);
> > > + regmap_read(regmap, CSI_CH_INT_EN_REG, );
> > > + printk("CSI_CH_INT_EN_REG=0x%x\n",  val);
> > > + regmap_read(regmap, CSI_CH_INT_STA_REG, );
> > > + printk("CSI_CH_INT_STA_REG=0x%x\n", val);
> > > + regmap_read(regmap, CSI_CH_FLD1_VSIZE_REG, );
> > > + printk("CSI_CH_FLD1_VSIZE_REG=0x%x\n",  val);
> > > + regmap_read(regmap, CSI_CH_HSIZE_REG, );
> > > + printk("CSI_CH_HSIZE_REG=0x%x\n",   val);
> > > + regmap_read(regmap, CSI_CH_VSIZE_REG, );
> > > + printk("CSI_CH_VSIZE_REG=0x%x\n",   val);
> > > + regmap_read(regmap, CSI_CH_BUF_LEN_REG, );
> > > + printk("CSI_CH_BUF_LEN_REG=0x%x\n", val);
> > > + regmap_read(regmap, CSI_CH_FLIP_SIZE_REG, );
> > > + printk("CSI_CH_FLIP_SIZE_REG=0x%x\n",   val);
> > > + regmap_read(regmap, CSI_CH_FRM_CLK_CNT_REG, );
> > > + printk("CSI_CH_FRM_CLK_CNT_REG=0x%x\n", val);
> > > + regmap_read(regmap, CSI_CH_ACC_ITNL_CLK_CNT_REG, );
> > > + printk("CSI_CH_ACC_ITNL_CLK_CNT_REG=0x%x\n",val);
> > > + regmap_read(regmap, CSI_CH_FIFO_STAT_REG, );
> > > + printk("CSI_CH_FIFO_STAT_REG=0x%x\n",   val);
> > > + regmap_read(regmap, CSI_CH_PCLK_STAT_REG, );
> > > + printk("CSI_CH_PCLK_STAT_REG=0x%x\n",   val);
> > > +}
> > > +#endif
> > 
> > You can already dump a regmap through debugfs, that's redundant.
> 
> The advantage of in-code registers dump routine is the ability to
> synchronize the snapshot with the driver code execution. This is
> particularly important for the capture statistics registers. I have
> found it useful here.

You also have the option to use the traces to do that, but if that's
useful, this should be added to regmap itself. It can benefit others
too.

> > > +static irqreturn_t sun6i_csi_isr(int irq, void *dev_id)
> > > +{
> > > + struct sun6i_csi_dev *sdev = (struct sun6i_csi_dev *)dev_id;
> > > + struct regmap *regmap = sdev->regmap;
> > > + u32 status;
> > > +
> > > + regmap_read(regmap, CSI_CH_INT_STA_REG, );
> > > +
> > > + if ((status & CSI_CH_INT_STA_FIFO0_OF_PD) ||
> > > + (status & CSI_CH_INT_STA_FIFO1_OF_PD) ||
> > > + (status & CSI_CH_INT_STA_FIFO2_OF_PD) ||
> > > + (status & CSI_CH_INT_STA_HB_OF_PD)) {
> > > + regmap_write(regmap, CSI_CH_INT_STA_REG, status);
> > > + regmap_update_bits(regmap, CSI_EN_REG, CSI_EN_CSI_EN, 0);
> > > + regmap_update_bits(regmap, CSI_EN_REG, CSI_EN_CSI_EN,
> > > +CSI_EN_CSI_EN);
> > 
> > You need to enable / disable it at every frame? How do you deal with
> > double buffering? (or did you choose to ignore it for now?)
> 
> These *_OF_PD status bits indicate an overflow error condition.

Shouldn't we return an error code then? The names of these flags 

Re: [PATCH v2 1/3] media: V3s: Add support for Allwinner CSI.

[Hans Verkuil]

Hi Yong,

First two high-level comments before I start the review:

1) Can you provide the v4l2-compliance output? I can't merge this unless I
   see that it passes the compliance tests. Make sure you compile from the git
   repo (https://git.linuxtv.org/v4l-utils.git/) so you are using the latest
   version of the compliance test. Test with just 'v4l2-compliance' and also
   with the -s option (to test streaming) and with the -f option (to test all
   the various pixel formats).

2) I see you support interlaced formats. Did you actually test that? Interlaced
   is tricky and you shouldn't add support for it unless you know it works and
   that it passes the 'v4l2-compliance -s' test.

On 07/27/2017 07:01 AM, Yong Deng wrote:
> Allwinner V3s SoC have two CSI module. CSI0 is used for MIPI interface
> and CSI1 is used for parallel interface. This is not documented in
> datasheet but by testing and guess.
> 
> This patch implement a v4l2 framework driver for it.
> 
> Currently, the driver only support the parallel interface. MIPI-CSI2,
> ISP's support are not included in this patch.
> 
> Signed-off-by: Yong Deng 
> ---
>  drivers/media/platform/Kconfig   |   1 +
>  drivers/media/platform/Makefile  |   2 +
>  drivers/media/platform/sun6i-csi/Kconfig |   9 +
>  drivers/media/platform/sun6i-csi/Makefile|   3 +
>  drivers/media/platform/sun6i-csi/sun6i_csi.c | 545 +++
>  drivers/media/platform/sun6i-csi/sun6i_csi.h | 203 ++
>  drivers/media/platform/sun6i-csi/sun6i_csi_v3s.c | 827 
> +++
>  drivers/media/platform/sun6i-csi/sun6i_csi_v3s.h | 206 ++
>  drivers/media/platform/sun6i-csi/sun6i_video.c   | 663 ++
>  drivers/media/platform/sun6i-csi/sun6i_video.h   |  61 ++
>  10 files changed, 2520 insertions(+)
>  create mode 100644 drivers/media/platform/sun6i-csi/Kconfig
>  create mode 100644 drivers/media/platform/sun6i-csi/Makefile
>  create mode 100644 drivers/media/platform/sun6i-csi/sun6i_csi.c
>  create mode 100644 drivers/media/platform/sun6i-csi/sun6i_csi.h
>  create mode 100644 drivers/media/platform/sun6i-csi/sun6i_csi_v3s.c
>  create mode 100644 drivers/media/platform/sun6i-csi/sun6i_csi_v3s.h
>  create mode 100644 drivers/media/platform/sun6i-csi/sun6i_video.c
>  create mode 100644 drivers/media/platform/sun6i-csi/sun6i_video.h
> 



> diff --git a/drivers/media/platform/sun6i-csi/sun6i_video.c 
> b/drivers/media/platform/sun6i-csi/sun6i_video.c
> new file mode 100644
> index 000..0c5dbd2
> --- /dev/null
> +++ b/drivers/media/platform/sun6i-csi/sun6i_video.c
> @@ -0,0 +1,663 @@
> +/*
> + * Copyright (c) 2017 Magewell Electronics Co., Ltd. (Nanjing).
> + * All rights reserved.
> + * Author: Yong Deng 
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include 
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include "sun6i_csi.h"
> +#include "sun6i_video.h"
> +
> +struct sun6i_csi_buffer {
> + struct vb2_v4l2_buffer  vb;
> + struct list_headlist;
> +
> + dma_addr_t  dma_addr;
> +};
> +
> +static struct sun6i_csi_format *
> +find_format_by_fourcc(struct sun6i_video *video, unsigned int fourcc)
> +{
> + unsigned int num_formats = video->num_formats;
> + struct sun6i_csi_format *fmt;
> + unsigned int i;
> +
> + for (i = 0; i < num_formats; i++) {
> + fmt = >formats[i];
> + if (fmt->fourcc == fourcc)
> + return fmt;
> + }
> +
> + return NULL;
> +}
> +
> +static struct v4l2_subdev *
> +sun6i_video_remote_subdev(struct sun6i_video *video, u32 *pad)
> +{
> + struct media_pad *remote;
> +
> + remote = media_entity_remote_pad(>pad);
> +
> + if (!remote || !is_media_entity_v4l2_subdev(remote->entity))
> + return NULL;
> +
> + if (pad)
> + *pad = remote->index;
> +
> + return media_entity_to_v4l2_subdev(remote->entity);
> +}
> +
> +static int sun6i_video_queue_setup(struct vb2_queue *vq,
> +  unsigned int *nbuffers, unsigned int *nplanes,
> +  unsigned int sizes[],
> +  struct device *alloc_devs[])
> +{
> + struct sun6i_video *video = vb2_get_drv_priv(vq);
> + unsigned int size = video->fmt.fmt.pix.sizeimage;
> +
> + if (*nplanes)
> + return sizes[0] < size ? -EINVAL : 0;
> +
> + *nplanes = 1;
> + sizes[0] = size;
> +
> + 

Re: [PATCH v2 1/3] media: V3s: Add support for Allwinner CSI.

[Baruch Siach]

Hi Yong,

On Mon, Jul 31, 2017 at 09:48:06AM +0800, Yong wrote:
> On Sun, 30 Jul 2017 09:08:01 +0300
> Baruch Siach  wrote:
> > On Fri, Jul 28, 2017 at 06:02:33PM +0200, Maxime Ripard wrote:
> > > On Thu, Jul 27, 2017 at 01:01:35PM +0800, Yong Deng wrote:
> > > > +   regmap_write(sdev->regmap, CSI_CH_F0_BUFA_REG,
> > > > +(bus_addr + sdev->planar_offset[0]) >> 2);
> > 
> > Why do you need the bit shift? Does that work for you?
> > 
> > The User Manuals of both the V3s and the and the A33 (AKA R16) state that 
> > the 
> > BUFA field size in this register is 31:00, that is 32bit. I have found no 
> > indication of this bit shift in the Olimex provided sunxi-vfe[1] driver. On 
> > the A33 I have found that only after removing the bit-shift, (some sort of) 
> > data started to appear in the buffer.
> > 
> > [1] 
> > https://github.com/hehopmajieh/a33_linux/tree/master/drivers/media/video/sunxi-vfe
> 
> The Users Manuals do not document this bit shift. You should see line 10 to
> 32 in 
> https://github.com/hehopmajieh/a33_linux/blob/master/drivers/media/video/sunxi-vfe/csi/csi_reg.c

Thanks. So for my reference, the SoCs that don't need bit shift are A31, A23, 
and A33. SoCs that need bit shift are A80, A83, H3, and V3s (AKA V30).

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 v2 1/3] media: V3s: Add support for Allwinner CSI.

[Yong]

Hi,

On Fri, 28 Jul 2017 18:02:33 +0200
Maxime Ripard  wrote:

> Hi, 
> 
> Thanks for the second iteration!
> 
> On Thu, Jul 27, 2017 at 01:01:35PM +0800, Yong Deng wrote:
> > Allwinner V3s SoC have two CSI module. CSI0 is used for MIPI interface
> > and CSI1 is used for parallel interface. This is not documented in
> > datasheet but by testing and guess.
> > 
> > This patch implement a v4l2 framework driver for it.
> > 
> > Currently, the driver only support the parallel interface. MIPI-CSI2,
> > ISP's support are not included in this patch.
> > 
> > Signed-off-by: Yong Deng 
> 
> There's a significant amount of checkpatch warnings (and quite
> important checks) in your driver. You should fix everything checkpatch
> --strict reports.

OK. I will check and fix.

> 
> > ---
> >  drivers/media/platform/Kconfig   |   1 +
> >  drivers/media/platform/Makefile  |   2 +
> >  drivers/media/platform/sun6i-csi/Kconfig |   9 +
> >  drivers/media/platform/sun6i-csi/Makefile|   3 +
> >  drivers/media/platform/sun6i-csi/sun6i_csi.c | 545 +++
> >  drivers/media/platform/sun6i-csi/sun6i_csi.h | 203 ++
> >  drivers/media/platform/sun6i-csi/sun6i_csi_v3s.c | 827 
> > +++
> >  drivers/media/platform/sun6i-csi/sun6i_csi_v3s.h | 206 ++
> >  drivers/media/platform/sun6i-csi/sun6i_video.c   | 663 ++
> >  drivers/media/platform/sun6i-csi/sun6i_video.h   |  61 ++
> >  10 files changed, 2520 insertions(+)
> >  create mode 100644 drivers/media/platform/sun6i-csi/Kconfig
> >  create mode 100644 drivers/media/platform/sun6i-csi/Makefile
> >  create mode 100644 drivers/media/platform/sun6i-csi/sun6i_csi.c
> >  create mode 100644 drivers/media/platform/sun6i-csi/sun6i_csi.h
> >  create mode 100644 drivers/media/platform/sun6i-csi/sun6i_csi_v3s.c
> >  create mode 100644 drivers/media/platform/sun6i-csi/sun6i_csi_v3s.h
> >  create mode 100644 drivers/media/platform/sun6i-csi/sun6i_video.c
> >  create mode 100644 drivers/media/platform/sun6i-csi/sun6i_video.h
> > 
> > diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
> > index 0c741d1..8371a87 100644
> > --- a/drivers/media/platform/Kconfig
> > +++ b/drivers/media/platform/Kconfig
> > @@ -143,6 +143,7 @@ source "drivers/media/platform/am437x/Kconfig"
> >  source "drivers/media/platform/xilinx/Kconfig"
> >  source "drivers/media/platform/rcar-vin/Kconfig"
> >  source "drivers/media/platform/atmel/Kconfig"
> > +source "drivers/media/platform/sun6i-csi/Kconfig"
> 
> We're going to have several different drivers in v4l eventually, so I
> guess it would make sense to move to a directory of our own.

Like this?
drivers/media/platform/sunxi/sun6i-csi

> 
> > +   dev_dbg(csi->dev, "creating links for entity %s\n", local->name);
> > +
> > +   while (1) {
> > +   /* Get the next endpoint and parse its link. */
> > +   next = of_graph_get_next_endpoint(entity->node, ep);
> > +   if (next == NULL)
> > +   break;
> > +
> > +   of_node_put(ep);
> > +   ep = next;
> > +
> > +   dev_dbg(csi->dev, "processing endpoint %s\n", ep->full_name);
> > +
> > +   ret = v4l2_fwnode_parse_link(of_fwnode_handle(ep), );
> > +   if (ret < 0) {
> > +   dev_err(csi->dev, "failed to parse link for %s\n",
> > +   ep->full_name);
> > +   continue;
> > +   }
> > +
> > +   /* Skip sink ports, they will be processed from the other end of
> > +* the link.
> > +*/
> > +   if (link.local_port >= local->num_pads) {
> > +   dev_err(csi->dev, "invalid port number %u on %s\n",
> > +   link.local_port,
> > +   to_of_node(link.local_node)->full_name);
> > +   v4l2_fwnode_put_link();
> > +   ret = -EINVAL;
> > +   break;
> > +   }
> > +
> > +   local_pad = >pads[link.local_port];
> > +
> > +   if (local_pad->flags & MEDIA_PAD_FL_SINK) {
> > +   dev_dbg(csi->dev, "skipping sink port %s:%u\n",
> > +   to_of_node(link.local_node)->full_name,
> > +   link.local_port);
> > +   v4l2_fwnode_put_link();
> > +   continue;
> > +   }
> > +
> > +   /* Skip video node, they will be processed separately. */
> > +   if (link.remote_node == of_fwnode_handle(csi->dev->of_node)) {
> > +   dev_dbg(csi->dev, "skipping CSI port %s:%u\n",
> > +   to_of_node(link.local_node)->full_name,
> > +   link.local_port);
> > +   v4l2_fwnode_put_link();
> > +   continue;
> > +   }
> > +
> > +   /* Find the remote entity. */
> > +  

Re: [PATCH v2 1/3] media: V3s: Add support for Allwinner CSI.

[Yong]

Hi,

On Sun, 30 Jul 2017 09:08:01 +0300
Baruch Siach  wrote:

> Hi Maxime, Yong,
> 
> On Fri, Jul 28, 2017 at 06:02:33PM +0200, Maxime Ripard wrote:
> > Hi, 
> > 
> > Thanks for the second iteration!
> > 
> > On Thu, Jul 27, 2017 at 01:01:35PM +0800, Yong Deng wrote:
> > > Allwinner V3s SoC have two CSI module. CSI0 is used for MIPI interface
> > > and CSI1 is used for parallel interface. This is not documented in
> > > datasheet but by testing and guess.
> > > 
> > > This patch implement a v4l2 framework driver for it.
> > > 
> > > Currently, the driver only support the parallel interface. MIPI-CSI2,
> > > ISP's support are not included in this patch.
> > > 
> > > Signed-off-by: Yong Deng 
> 
> [...]
> 
> > > +#ifdef DEBUG
> > > +static void sun6i_csi_dump_regs(struct sun6i_csi_dev *sdev)
> > > +{
> > > + struct regmap *regmap = sdev->regmap;
> > > + u32 val;
> > > +
> > > + regmap_read(regmap, CSI_EN_REG, );
> > > + printk("CSI_EN_REG=0x%x\n", val);
> > > + regmap_read(regmap, CSI_IF_CFG_REG, );
> > > + printk("CSI_IF_CFG_REG=0x%x\n", val);
> > > + regmap_read(regmap, CSI_CAP_REG, );
> > > + printk("CSI_CAP_REG=0x%x\n",val);
> > > + regmap_read(regmap, CSI_SYNC_CNT_REG, );
> > > + printk("CSI_SYNC_CNT_REG=0x%x\n",   val);
> > > + regmap_read(regmap, CSI_FIFO_THRS_REG, );
> > > + printk("CSI_FIFO_THRS_REG=0x%x\n",  val);
> > > + regmap_read(regmap, CSI_PTN_LEN_REG, );
> > > + printk("CSI_PTN_LEN_REG=0x%x\n",val);
> > > + regmap_read(regmap, CSI_PTN_ADDR_REG, );
> > > + printk("CSI_PTN_ADDR_REG=0x%x\n",   val);
> > > + regmap_read(regmap, CSI_VER_REG, );
> > > + printk("CSI_VER_REG=0x%x\n",val);
> > > + regmap_read(regmap, CSI_CH_CFG_REG, );
> > > + printk("CSI_CH_CFG_REG=0x%x\n", val);
> > > + regmap_read(regmap, CSI_CH_SCALE_REG, );
> > > + printk("CSI_CH_SCALE_REG=0x%x\n",   val);
> > > + regmap_read(regmap, CSI_CH_F0_BUFA_REG, );
> > > + printk("CSI_CH_F0_BUFA_REG=0x%x\n", val);
> > > + regmap_read(regmap, CSI_CH_F1_BUFA_REG, );
> > > + printk("CSI_CH_F1_BUFA_REG=0x%x\n", val);
> > > + regmap_read(regmap, CSI_CH_F2_BUFA_REG, );
> > > + printk("CSI_CH_F2_BUFA_REG=0x%x\n", val);
> > > + regmap_read(regmap, CSI_CH_STA_REG, );
> > > + printk("CSI_CH_STA_REG=0x%x\n", val);
> > > + regmap_read(regmap, CSI_CH_INT_EN_REG, );
> > > + printk("CSI_CH_INT_EN_REG=0x%x\n",  val);
> > > + regmap_read(regmap, CSI_CH_INT_STA_REG, );
> > > + printk("CSI_CH_INT_STA_REG=0x%x\n", val);
> > > + regmap_read(regmap, CSI_CH_FLD1_VSIZE_REG, );
> > > + printk("CSI_CH_FLD1_VSIZE_REG=0x%x\n",  val);
> > > + regmap_read(regmap, CSI_CH_HSIZE_REG, );
> > > + printk("CSI_CH_HSIZE_REG=0x%x\n",   val);
> > > + regmap_read(regmap, CSI_CH_VSIZE_REG, );
> > > + printk("CSI_CH_VSIZE_REG=0x%x\n",   val);
> > > + regmap_read(regmap, CSI_CH_BUF_LEN_REG, );
> > > + printk("CSI_CH_BUF_LEN_REG=0x%x\n", val);
> > > + regmap_read(regmap, CSI_CH_FLIP_SIZE_REG, );
> > > + printk("CSI_CH_FLIP_SIZE_REG=0x%x\n",   val);
> > > + regmap_read(regmap, CSI_CH_FRM_CLK_CNT_REG, );
> > > + printk("CSI_CH_FRM_CLK_CNT_REG=0x%x\n", val);
> > > + regmap_read(regmap, CSI_CH_ACC_ITNL_CLK_CNT_REG, );
> > > + printk("CSI_CH_ACC_ITNL_CLK_CNT_REG=0x%x\n",val);
> > > + regmap_read(regmap, CSI_CH_FIFO_STAT_REG, );
> > > + printk("CSI_CH_FIFO_STAT_REG=0x%x\n",   val);
> > > + regmap_read(regmap, CSI_CH_PCLK_STAT_REG, );
> > > + printk("CSI_CH_PCLK_STAT_REG=0x%x\n",   val);
> > > +}
> > > +#endif
> > 
> > You can already dump a regmap through debugfs, that's redundant.
> 
> The advantage of in-code registers dump routine is the ability to synchronize 
> the snapshot with the driver code execution. This is particularly important 
> for the capture statistics registers. I have found it useful here.

Agree. It is not used to expose the registers value to user space. If you
think it is redundant, I will delete it.

> 
> [...]
> 
> > > +static int update_buf_addr(struct sun6i_csi *csi, dma_addr_t addr)
> > > +{
> > > + struct sun6i_csi_dev *sdev = sun6i_csi_to_dev(csi);
> > > + /* transform physical address to bus address */
> > > + dma_addr_t bus_addr = addr - 0x4000;
> > 
> > Like Baruch noticed, you should use PHYS_OFFSET here. The A80 for
> > example has a different RAM base address.
> > 
> > > +
> > > + regmap_write(sdev->regmap, CSI_CH_F0_BUFA_REG,
> > > +  (bus_addr + sdev->planar_offset[0]) >> 2);
> 
> Why do you need the bit shift? Does that work for you?
> 
> The User Manuals of both the V3s and the and the A33 (AKA R16) state that the 
> BUFA field size in this register is 31:00, that is 32bit. I have found no 
> indication of this bit shift in the Olimex provided sunxi-vfe[1] driver. On 
> the A33 I have found that only after removing the bit-shift, (some sort of) 
> data started to appear in the buffer.
> 
> [1] 
> 

Re: [PATCH v2 1/3] media: V3s: Add support for Allwinner CSI.

[Yong]

On Thu, 27 Jul 2017 14:25:51 +0200
Maxime Ripard  wrote:

> On Thu, Jul 27, 2017 at 03:16:44PM +0300, Baruch Siach wrote:
> > Hi Yong,
> > 
> > I managed to get the Frame Done interrupt with the previous version of this 
> > driver on the A33 OLinuXino. No data yet (all zeros). I'm still working on 
> > it.
> > 
> > One comment below.
> > 
> > On Thu, Jul 27, 2017 at 01:01:35PM +0800, Yong Deng wrote:
> > > Allwinner V3s SoC have two CSI module. CSI0 is used for MIPI interface
> > > and CSI1 is used for parallel interface. This is not documented in
> > > datasheet but by testing and guess.
> > > 
> > > This patch implement a v4l2 framework driver for it.
> > > 
> > > Currently, the driver only support the parallel interface. MIPI-CSI2,
> > > ISP's support are not included in this patch.
> > > 
> > > Signed-off-by: Yong Deng 
> > > ---
> > 
> > [...]
> > 
> > > +static int update_buf_addr(struct sun6i_csi *csi, dma_addr_t addr)
> > > +{
> > > + struct sun6i_csi_dev *sdev = sun6i_csi_to_dev(csi);
> > > + /* transform physical address to bus address */
> > > + dma_addr_t bus_addr = addr - 0x4000;
> > 
> > What is the source of this magic number? Is it platform dependent? Are 
> > there 
> > other devices doing DMA that need this adjustment?
> 
> This is the RAM base address in most (but not all) Allwinner
> SoCs. You'll want to use PHYS_OFFSET instead.

I have try to use PHYS_OFFSET. But I found it is not 0x4000. I will
try it again.

> 
> Maxime
> 
> -- 
> Maxime Ripard, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com


Thanks,
Yong

Re: [PATCH v2 1/3] media: V3s: Add support for Allwinner CSI.

[Baruch Siach]

Hi Maxime, Yong,

On Fri, Jul 28, 2017 at 06:02:33PM +0200, Maxime Ripard wrote:
> Hi, 
> 
> Thanks for the second iteration!
> 
> On Thu, Jul 27, 2017 at 01:01:35PM +0800, Yong Deng wrote:
> > Allwinner V3s SoC have two CSI module. CSI0 is used for MIPI interface
> > and CSI1 is used for parallel interface. This is not documented in
> > datasheet but by testing and guess.
> > 
> > This patch implement a v4l2 framework driver for it.
> > 
> > Currently, the driver only support the parallel interface. MIPI-CSI2,
> > ISP's support are not included in this patch.
> > 
> > Signed-off-by: Yong Deng 

[...]

> > +#ifdef DEBUG
> > +static void sun6i_csi_dump_regs(struct sun6i_csi_dev *sdev)
> > +{
> > +   struct regmap *regmap = sdev->regmap;
> > +   u32 val;
> > +
> > +   regmap_read(regmap, CSI_EN_REG, );
> > +   printk("CSI_EN_REG=0x%x\n", val);
> > +   regmap_read(regmap, CSI_IF_CFG_REG, );
> > +   printk("CSI_IF_CFG_REG=0x%x\n", val);
> > +   regmap_read(regmap, CSI_CAP_REG, );
> > +   printk("CSI_CAP_REG=0x%x\n",val);
> > +   regmap_read(regmap, CSI_SYNC_CNT_REG, );
> > +   printk("CSI_SYNC_CNT_REG=0x%x\n",   val);
> > +   regmap_read(regmap, CSI_FIFO_THRS_REG, );
> > +   printk("CSI_FIFO_THRS_REG=0x%x\n",  val);
> > +   regmap_read(regmap, CSI_PTN_LEN_REG, );
> > +   printk("CSI_PTN_LEN_REG=0x%x\n",val);
> > +   regmap_read(regmap, CSI_PTN_ADDR_REG, );
> > +   printk("CSI_PTN_ADDR_REG=0x%x\n",   val);
> > +   regmap_read(regmap, CSI_VER_REG, );
> > +   printk("CSI_VER_REG=0x%x\n",val);
> > +   regmap_read(regmap, CSI_CH_CFG_REG, );
> > +   printk("CSI_CH_CFG_REG=0x%x\n", val);
> > +   regmap_read(regmap, CSI_CH_SCALE_REG, );
> > +   printk("CSI_CH_SCALE_REG=0x%x\n",   val);
> > +   regmap_read(regmap, CSI_CH_F0_BUFA_REG, );
> > +   printk("CSI_CH_F0_BUFA_REG=0x%x\n", val);
> > +   regmap_read(regmap, CSI_CH_F1_BUFA_REG, );
> > +   printk("CSI_CH_F1_BUFA_REG=0x%x\n", val);
> > +   regmap_read(regmap, CSI_CH_F2_BUFA_REG, );
> > +   printk("CSI_CH_F2_BUFA_REG=0x%x\n", val);
> > +   regmap_read(regmap, CSI_CH_STA_REG, );
> > +   printk("CSI_CH_STA_REG=0x%x\n", val);
> > +   regmap_read(regmap, CSI_CH_INT_EN_REG, );
> > +   printk("CSI_CH_INT_EN_REG=0x%x\n",  val);
> > +   regmap_read(regmap, CSI_CH_INT_STA_REG, );
> > +   printk("CSI_CH_INT_STA_REG=0x%x\n", val);
> > +   regmap_read(regmap, CSI_CH_FLD1_VSIZE_REG, );
> > +   printk("CSI_CH_FLD1_VSIZE_REG=0x%x\n",  val);
> > +   regmap_read(regmap, CSI_CH_HSIZE_REG, );
> > +   printk("CSI_CH_HSIZE_REG=0x%x\n",   val);
> > +   regmap_read(regmap, CSI_CH_VSIZE_REG, );
> > +   printk("CSI_CH_VSIZE_REG=0x%x\n",   val);
> > +   regmap_read(regmap, CSI_CH_BUF_LEN_REG, );
> > +   printk("CSI_CH_BUF_LEN_REG=0x%x\n", val);
> > +   regmap_read(regmap, CSI_CH_FLIP_SIZE_REG, );
> > +   printk("CSI_CH_FLIP_SIZE_REG=0x%x\n",   val);
> > +   regmap_read(regmap, CSI_CH_FRM_CLK_CNT_REG, );
> > +   printk("CSI_CH_FRM_CLK_CNT_REG=0x%x\n", val);
> > +   regmap_read(regmap, CSI_CH_ACC_ITNL_CLK_CNT_REG, );
> > +   printk("CSI_CH_ACC_ITNL_CLK_CNT_REG=0x%x\n",val);
> > +   regmap_read(regmap, CSI_CH_FIFO_STAT_REG, );
> > +   printk("CSI_CH_FIFO_STAT_REG=0x%x\n",   val);
> > +   regmap_read(regmap, CSI_CH_PCLK_STAT_REG, );
> > +   printk("CSI_CH_PCLK_STAT_REG=0x%x\n",   val);
> > +}
> > +#endif
> 
> You can already dump a regmap through debugfs, that's redundant.

The advantage of in-code registers dump routine is the ability to synchronize 
the snapshot with the driver code execution. This is particularly important 
for the capture statistics registers. I have found it useful here.

[...]

> > +static int update_buf_addr(struct sun6i_csi *csi, dma_addr_t addr)
> > +{
> > +   struct sun6i_csi_dev *sdev = sun6i_csi_to_dev(csi);
> > +   /* transform physical address to bus address */
> > +   dma_addr_t bus_addr = addr - 0x4000;
> 
> Like Baruch noticed, you should use PHYS_OFFSET here. The A80 for
> example has a different RAM base address.
> 
> > +
> > +   regmap_write(sdev->regmap, CSI_CH_F0_BUFA_REG,
> > +(bus_addr + sdev->planar_offset[0]) >> 2);

Why do you need the bit shift? Does that work for you?

The User Manuals of both the V3s and the and the A33 (AKA R16) state that the 
BUFA field size in this register is 31:00, that is 32bit. I have found no 
indication of this bit shift in the Olimex provided sunxi-vfe[1] driver. On 
the A33 I have found that only after removing the bit-shift, (some sort of) 
data started to appear in the buffer.

[1] 
https://github.com/hehopmajieh/a33_linux/tree/master/drivers/media/video/sunxi-vfe

[...]

> > +static irqreturn_t sun6i_csi_isr(int irq, void *dev_id)
> > +{
> > +   struct sun6i_csi_dev *sdev = (struct sun6i_csi_dev *)dev_id;
> > +   struct regmap *regmap = sdev->regmap;
> > +   u32 status;
> > +
> > +   regmap_read(regmap, CSI_CH_INT_STA_REG, );
> > +
> > +   if ((status & 

Re: [PATCH v2 1/3] media: V3s: Add support for Allwinner CSI.

[Maxime Ripard]

Hi, 

Thanks for the second iteration!

On Thu, Jul 27, 2017 at 01:01:35PM +0800, Yong Deng wrote:
> Allwinner V3s SoC have two CSI module. CSI0 is used for MIPI interface
> and CSI1 is used for parallel interface. This is not documented in
> datasheet but by testing and guess.
> 
> This patch implement a v4l2 framework driver for it.
> 
> Currently, the driver only support the parallel interface. MIPI-CSI2,
> ISP's support are not included in this patch.
> 
> Signed-off-by: Yong Deng 

There's a significant amount of checkpatch warnings (and quite
important checks) in your driver. You should fix everything checkpatch
--strict reports.

> ---
>  drivers/media/platform/Kconfig   |   1 +
>  drivers/media/platform/Makefile  |   2 +
>  drivers/media/platform/sun6i-csi/Kconfig |   9 +
>  drivers/media/platform/sun6i-csi/Makefile|   3 +
>  drivers/media/platform/sun6i-csi/sun6i_csi.c | 545 +++
>  drivers/media/platform/sun6i-csi/sun6i_csi.h | 203 ++
>  drivers/media/platform/sun6i-csi/sun6i_csi_v3s.c | 827 
> +++
>  drivers/media/platform/sun6i-csi/sun6i_csi_v3s.h | 206 ++
>  drivers/media/platform/sun6i-csi/sun6i_video.c   | 663 ++
>  drivers/media/platform/sun6i-csi/sun6i_video.h   |  61 ++
>  10 files changed, 2520 insertions(+)
>  create mode 100644 drivers/media/platform/sun6i-csi/Kconfig
>  create mode 100644 drivers/media/platform/sun6i-csi/Makefile
>  create mode 100644 drivers/media/platform/sun6i-csi/sun6i_csi.c
>  create mode 100644 drivers/media/platform/sun6i-csi/sun6i_csi.h
>  create mode 100644 drivers/media/platform/sun6i-csi/sun6i_csi_v3s.c
>  create mode 100644 drivers/media/platform/sun6i-csi/sun6i_csi_v3s.h
>  create mode 100644 drivers/media/platform/sun6i-csi/sun6i_video.c
>  create mode 100644 drivers/media/platform/sun6i-csi/sun6i_video.h
> 
> diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
> index 0c741d1..8371a87 100644
> --- a/drivers/media/platform/Kconfig
> +++ b/drivers/media/platform/Kconfig
> @@ -143,6 +143,7 @@ source "drivers/media/platform/am437x/Kconfig"
>  source "drivers/media/platform/xilinx/Kconfig"
>  source "drivers/media/platform/rcar-vin/Kconfig"
>  source "drivers/media/platform/atmel/Kconfig"
> +source "drivers/media/platform/sun6i-csi/Kconfig"

We're going to have several different drivers in v4l eventually, so I
guess it would make sense to move to a directory of our own.

> + dev_dbg(csi->dev, "creating links for entity %s\n", local->name);
> +
> + while (1) {
> + /* Get the next endpoint and parse its link. */
> + next = of_graph_get_next_endpoint(entity->node, ep);
> + if (next == NULL)
> + break;
> +
> + of_node_put(ep);
> + ep = next;
> +
> + dev_dbg(csi->dev, "processing endpoint %s\n", ep->full_name);
> +
> + ret = v4l2_fwnode_parse_link(of_fwnode_handle(ep), );
> + if (ret < 0) {
> + dev_err(csi->dev, "failed to parse link for %s\n",
> + ep->full_name);
> + continue;
> + }
> +
> + /* Skip sink ports, they will be processed from the other end of
> +  * the link.
> +  */
> + if (link.local_port >= local->num_pads) {
> + dev_err(csi->dev, "invalid port number %u on %s\n",
> + link.local_port,
> + to_of_node(link.local_node)->full_name);
> + v4l2_fwnode_put_link();
> + ret = -EINVAL;
> + break;
> + }
> +
> + local_pad = >pads[link.local_port];
> +
> + if (local_pad->flags & MEDIA_PAD_FL_SINK) {
> + dev_dbg(csi->dev, "skipping sink port %s:%u\n",
> + to_of_node(link.local_node)->full_name,
> + link.local_port);
> + v4l2_fwnode_put_link();
> + continue;
> + }
> +
> + /* Skip video node, they will be processed separately. */
> + if (link.remote_node == of_fwnode_handle(csi->dev->of_node)) {
> + dev_dbg(csi->dev, "skipping CSI port %s:%u\n",
> + to_of_node(link.local_node)->full_name,
> + link.local_port);
> + v4l2_fwnode_put_link();
> + continue;
> + }
> +
> + /* Find the remote entity. */
> + ent = sun6i_graph_find_entity(csi,
> +   to_of_node(link.remote_node));
> + if (ent == NULL) {
> + dev_err(csi->dev, "no entity found for %s\n",
> + to_of_node(link.remote_node)->full_name);
> +  

Re: [PATCH v2 1/3] media: V3s: Add support for Allwinner CSI.

[Maxime Ripard]

On Thu, Jul 27, 2017 at 03:16:44PM +0300, Baruch Siach wrote:
> Hi Yong,
> 
> I managed to get the Frame Done interrupt with the previous version of this 
> driver on the A33 OLinuXino. No data yet (all zeros). I'm still working on it.
> 
> One comment below.
> 
> On Thu, Jul 27, 2017 at 01:01:35PM +0800, Yong Deng wrote:
> > Allwinner V3s SoC have two CSI module. CSI0 is used for MIPI interface
> > and CSI1 is used for parallel interface. This is not documented in
> > datasheet but by testing and guess.
> > 
> > This patch implement a v4l2 framework driver for it.
> > 
> > Currently, the driver only support the parallel interface. MIPI-CSI2,
> > ISP's support are not included in this patch.
> > 
> > Signed-off-by: Yong Deng 
> > ---
> 
> [...]
> 
> > +static int update_buf_addr(struct sun6i_csi *csi, dma_addr_t addr)
> > +{
> > +   struct sun6i_csi_dev *sdev = sun6i_csi_to_dev(csi);
> > +   /* transform physical address to bus address */
> > +   dma_addr_t bus_addr = addr - 0x4000;
> 
> What is the source of this magic number? Is it platform dependent? Are there 
> other devices doing DMA that need this adjustment?

This is the RAM base address in most (but not all) Allwinner
SoCs. You'll want to use PHYS_OFFSET instead.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com


signature.asc
Description: PGP signature

Re: [PATCH v2 1/3] media: V3s: Add support for Allwinner CSI.

[Baruch Siach]

Hi Yong,

I managed to get the Frame Done interrupt with the previous version of this 
driver on the A33 OLinuXino. No data yet (all zeros). I'm still working on it.

One comment below.

On Thu, Jul 27, 2017 at 01:01:35PM +0800, Yong Deng wrote:
> Allwinner V3s SoC have two CSI module. CSI0 is used for MIPI interface
> and CSI1 is used for parallel interface. This is not documented in
> datasheet but by testing and guess.
> 
> This patch implement a v4l2 framework driver for it.
> 
> Currently, the driver only support the parallel interface. MIPI-CSI2,
> ISP's support are not included in this patch.
> 
> Signed-off-by: Yong Deng 
> ---

[...]

> +static int update_buf_addr(struct sun6i_csi *csi, dma_addr_t addr)
> +{
> + struct sun6i_csi_dev *sdev = sun6i_csi_to_dev(csi);
> + /* transform physical address to bus address */
> + dma_addr_t bus_addr = addr - 0x4000;

What is the source of this magic number? Is it platform dependent? Are there 
other devices doing DMA that need this adjustment?

baruch

> +
> + regmap_write(sdev->regmap, CSI_CH_F0_BUFA_REG,
> +  (bus_addr + sdev->planar_offset[0]) >> 2);
> + if (sdev->planar_offset[1] != -1)
> + regmap_write(sdev->regmap, CSI_CH_F1_BUFA_REG,
> +  (bus_addr + sdev->planar_offset[1]) >> 2);
> + if (sdev->planar_offset[2] != -1)
> + regmap_write(sdev->regmap, CSI_CH_F2_BUFA_REG,
> +  (bus_addr + sdev->planar_offset[2]) >> 2);
> +
> + 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 -