RE: [PATCH v7 00/16] Intel IPU3 ImgU patchset

2018-12-04 Thread Mani, Rajmohan
Hi Laurent,

> > Hi Rajmohan,
> >
> > On Tuesday, 4 December 2018 18:07:16 EET Mani, Rajmohan wrote:
> > > >> On Thursday, 29 November 2018 21:51:32 EET Tomasz Figa wrote:
> > > >>> On Thu, Nov 29, 2018 at 6:43 AM Laurent Pinchart wrote:
> > >  On Tuesday, 30 October 2018 00:22:54 EET Yong Zhi wrote:
> > > >>
> > > >> [snip]
> > > >>
> > > > 1. Link pad flag of video nodes (i.e. ipu3-imgu 0 output) need
> > > > to be enabled prior to the test.
> > > > 2. Stream tests are not performed since it requires
> > > > pre-configuration for each case.
> > > 
> > >  And that's a bit of an issue. I've tested the driver with a
> > >  small script based on media-ctl to configure links and yavta to
> > >  interface with the video nodes, and got the following oops:
> >
> > [snip]
> >
> > >  The script can be found at
> > >  https://lists.libcamera.org/pipermail/libcamera-devel/2018-Nove
> > >  mb
> > >  e
> > >  r/40.html.
> > > 
> > >  I may be doing something wrong (and I probably am), but in any
> > >  case, the driver shouldn't crash. Could you please have a look ?
> > > >>>
> > > >>> It looks like the driver doesn't have the default state
> > > >>> initialized correctly somewhere and it ends up using 0 as the
> > > >>> divisor in some calculation? Something to fix indeed.
> > > >>
> > > >> That's probably the case. I'll trust Intel to fix that in v8 :-)
> > > >
> > > > Ack.
> > >
> > > Thanks for catching this.
> > > I was able to reproduce this error and I see that error handling is
> > > missing, leading to the panic.
> > >
> > > https://git.linuxtv.org/sailus/media_tree.git/tree/drivers/media
> > > /pci/intel/ipu3/ipu3-css-params.c?h=ipu3-v7=
> > > 19cee7329ca2d0156043cac6afcde535e93310af#n433
> > >
> > > is where the -EINVAL is returned.
> > >
> > > Setting the return type as int for the following function and all
> > > its callers to use the return value properly to error out, makes the
> > > panic go away.
> >
> > I assume that I will still not be able to process frames through the
> > pipe then, as I'll get an error :-) Could you tell me why the
> > configuration is incorrect and how I can fix it ?
> >
> 
> :)
> Let me look into this more and get back.
> Thanks for your patience.

I can see a couple of steps missing in the script below.
(https://lists.libcamera.org/pipermail/libcamera-devel/2018-November/40.html)

>From patch 02 of this v7 series "doc-rst: Add Intel IPU3 documentation", under 
>section
"Configuring ImgU V4L2 subdev for image processing"...

1. The pipe mode needs to be configured for the V4L2 subdev.

Also the pipe mode of the corresponding V4L2 subdev should be set as 
desired (e.g 0 for video mode or 1 for still mode) through the control 
id 0x009819a1 as below.

e.g v4l2n -d /dev/v4l-subdev7 --ctrl=0x009819A1=1

2. ImgU pipeline needs to be configured for image processing as below.

RAW bayer frames go through the following ISP pipeline HW blocks to 
have the processed image output to the DDR memory.

RAW bayer frame -> Input Feeder -> Bayer Down Scaling (BDS) -> 
Geometric Distortion Correction (GDC) -> DDR

The ImgU V4L2 subdev has to be configured with the supported 
resolutions in all the above HW blocks, for a given input resolution.

For a given supported resolution for an input frame, the Input Feeder, 
Bayer Down Scaling and GDC blocks should be configured with the 
supported resolutions. This information can be obtained by looking at 
the following IPU3 ISP configuration table for ov5670 sensor.

https://chromium.googlesource.com/chromiumos/overlays/board-overlays/+/master
/baseboard-poppy/media-libs/cros-camera-hal-configs-poppy/files/gcss
/graph_settings_ov5670.xml

For the ov5670 example, for an input frame with a resolution of 
2592x1944 (which is input to the ImgU subdev pad 0), the corresponding 
resolutions for input feeder, BDS and GDC are 2592x1944, 2592x1944 and 
2560x1920 respectively.

The following steps prepare the ImgU ISP pipeline for the image processing.

1. The ImgU V4L2 subdev data format should be set by using the 
VIDIOC_SUBDEV_S_FMT on pad 0, using the GDC width and height obtained above.

2. The ImgU V4L2 subdev cropping should be set by using the 
VIDIOC_SUBDEV_S_SELECTION on pad 0, with V4L2_SEL_TGT_CROP as the 
target, using the input feeder height and width.

3. The ImgU V4L2 subdev composing should be set by using the 
VIDIOC_SUBDEV_S_SELECTION on pad 0, with V4L2_SEL_TGT_COMPOSE as the 
target, using the BDS height and width.

Once these 2 steps are done, the raw bayer frames can be input to the 
ImgU V4L2 subdev for processing.

> Raj
> 
> > > ipu3_css_osys_calc_frame_and_stripe_params()
> > >
> > > Will include the fix in v8.
> >
> > Thank you.
> >
> > > Thanks for catching this.
> >
> > You're welcome.
> >
> > --
> > Regards,
> >
> > Laurent Pinchart
> >
> >



RE: [PATCH v7 00/16] Intel IPU3 ImgU patchset

2018-12-04 Thread Mani, Rajmohan
Hi Laurent,

> -Original Message-
> From: Laurent Pinchart [mailto:laurent.pinch...@ideasonboard.com]
> Sent: Tuesday, December 04, 2018 8:42 AM
> To: Mani, Rajmohan 
> Cc: Tomasz Figa ; Zhi, Yong ;
> Linux Media Mailing List ; Sakari Ailus
> ; Mauro Carvalho Chehab
> ; Hans Verkuil ; Zheng, Jian
> Xu ; Hu, Jerry W ;
> Toivonen, Tuukka ; Qiu, Tian Shu
> ; Cao, Bingbu 
> Subject: Re: [PATCH v7 00/16] Intel IPU3 ImgU patchset
> 
> Hi Rajmohan,
> 
> On Tuesday, 4 December 2018 18:07:16 EET Mani, Rajmohan wrote:
> > >> On Thursday, 29 November 2018 21:51:32 EET Tomasz Figa wrote:
> > >>> On Thu, Nov 29, 2018 at 6:43 AM Laurent Pinchart wrote:
> > >>>> On Tuesday, 30 October 2018 00:22:54 EET Yong Zhi wrote:
> > >>
> > >> [snip]
> > >>
> > >>>>> 1. Link pad flag of video nodes (i.e. ipu3-imgu 0 output) need
> > >>>>> to be enabled prior to the test.
> > >>>>> 2. Stream tests are not performed since it requires
> > >>>>> pre-configuration for each case.
> > >>>>
> > >>>> And that's a bit of an issue. I've tested the driver with a small
> > >>>> script based on media-ctl to configure links and yavta to
> > >>>> interface with the video nodes, and got the following oops:
> 
> [snip]
> 
> > >>>> The script can be found at
> > >>>> https://lists.libcamera.org/pipermail/libcamera-devel/2018-Novemb
> > >>>> e
> > >>>> r/40.html.
> > >>>>
> > >>>> I may be doing something wrong (and I probably am), but in any
> > >>>> case, the driver shouldn't crash. Could you please have a look ?
> > >>>
> > >>> It looks like the driver doesn't have the default state
> > >>> initialized correctly somewhere and it ends up using 0 as the
> > >>> divisor in some calculation? Something to fix indeed.
> > >>
> > >> That's probably the case. I'll trust Intel to fix that in v8 :-)
> > >
> > > Ack.
> >
> > Thanks for catching this.
> > I was able to reproduce this error and I see that error handling is
> > missing, leading to the panic.
> >
> > https://git.linuxtv.org/sailus/media_tree.git/tree/drivers/media
> > /pci/intel/ipu3/ipu3-css-params.c?h=ipu3-v7=
> > 19cee7329ca2d0156043cac6afcde535e93310af#n433
> >
> > is where the -EINVAL is returned.
> >
> > Setting the return type as int for the following function and all its
> > callers to use the return value properly to error out, makes the panic
> > go away.
> 
> I assume that I will still not be able to process frames through the pipe 
> then, as
> I'll get an error :-) Could you tell me why the configuration is incorrect 
> and how
> I can fix it ?
> 

:)
Let me look into this more and get back.
Thanks for your patience.

Raj

> > ipu3_css_osys_calc_frame_and_stripe_params()
> >
> > Will include the fix in v8.
> 
> Thank you.
> 
> > Thanks for catching this.
> 
> You're welcome.
> 
> --
> Regards,
> 
> Laurent Pinchart
> 
> 



Re: [PATCH v7 00/16] Intel IPU3 ImgU patchset

2018-12-04 Thread Laurent Pinchart
Hi Rajmohan,

On Tuesday, 4 December 2018 18:07:16 EET Mani, Rajmohan wrote:
> >> On Thursday, 29 November 2018 21:51:32 EET Tomasz Figa wrote:
> >>> On Thu, Nov 29, 2018 at 6:43 AM Laurent Pinchart wrote:
>  On Tuesday, 30 October 2018 00:22:54 EET Yong Zhi wrote:
> >> 
> >> [snip]
> >> 
> > 1. Link pad flag of video nodes (i.e. ipu3-imgu 0 output) need to
> > be enabled prior to the test.
> > 2. Stream tests are not performed since it requires
> > pre-configuration for each case.
>  
>  And that's a bit of an issue. I've tested the driver with a small
>  script based on media-ctl to configure links and yavta to
>  interface with the video nodes, and got the following oops:

[snip]

>  The script can be found at
>  https://lists.libcamera.org/pipermail/libcamera-devel/2018-Novembe
>  r/40.html.
>  
>  I may be doing something wrong (and I probably am), but in any
>  case, the driver shouldn't crash. Could you please have a look ?
> >>> 
> >>> It looks like the driver doesn't have the default state initialized
> >>> correctly somewhere and it ends up using 0 as the divisor in some
> >>> calculation? Something to fix indeed.
> >> 
> >> That's probably the case. I'll trust Intel to fix that in v8 :-)
> > 
> > Ack.
> 
> Thanks for catching this.
> I was able to reproduce this error and I see that error handling
> is missing, leading to the panic.
> 
> https://git.linuxtv.org/sailus/media_tree.git/tree/drivers/media
> /pci/intel/ipu3/ipu3-css-params.c?h=ipu3-v7=
> 19cee7329ca2d0156043cac6afcde535e93310af#n433
> 
> is where the -EINVAL is returned.
> 
> Setting the return type as int for the following function and all
> its callers to use the return value properly to error out, makes
> the panic go away.

I assume that I will still not be able to process frames through the pipe 
then, as I'll get an error :-) Could you tell me why the configuration is 
incorrect and how I can fix it ?

> ipu3_css_osys_calc_frame_and_stripe_params()
> 
> Will include the fix in v8.

Thank you.

> Thanks for catching this.

You're welcome.

-- 
Regards,

Laurent Pinchart





RE: [PATCH v7 00/16] Intel IPU3 ImgU patchset

2018-12-04 Thread Mani, Rajmohan
Hi Laurent, Tomasz,

> 
> Thanks for the reviews.
> 
> > Subject: Re: [PATCH v7 00/16] Intel IPU3 ImgU patchset
> >
> > Hi Tomasz,
> >
> > On Thursday, 29 November 2018 21:51:32 EET Tomasz Figa wrote:
> > > On Thu, Nov 29, 2018 at 6:43 AM Laurent Pinchart wrote:
> > > > On Tuesday, 30 October 2018 00:22:54 EET Yong Zhi wrote:
> >
> > [snip]
> >
> > > >> 1. Link pad flag of video nodes (i.e. ipu3-imgu 0 output) need to
> > > >> be enabled prior to the test.
> > > >> 2. Stream tests are not performed since it requires
> > > >> pre-configuration for each case.
> > > >
> > > > And that's a bit of an issue. I've tested the driver with a small
> > > > script based on media-ctl to configure links and yavta to
> > > > interface with the video nodes, and got the following oops:
> > > >
> > > > [  136.927788] divide error:  [#1] PREEMPT SMP PTI [
> > > > 136.927801] CPU: 2 PID: 2069 Comm: yavta Not tainted 4.20.0-rc1+
> > > > #9 [  136.927806] Hardware name: HP Soraka/Soraka, BIOS
> > > > 08/30/2018 [ 136.927820] RIP: 0010:ipu3_css_osys_calc+0xc54/0xe14
> > > > [ipu3_imgu] [ 136.927825] Code: 89 44 24 28 42 8b 44 86 6c f7 54
> > > > 24 04 81 64 24 28
> > > > 00 fd ff ff 81 64 24 04 00 03 00 00 8d 44 03 ff 81 44 24 28 80 03
> > > > 00
> > > > 00 99  fb 0f af c3 bb 20 00 00 00 99 f7 fb 8b 5c 24 40 83 fd
> > > > 01
> > > > 19 d2 [  136.927830] RSP: 0018:9af2c0b837c8 EFLAGS: 00010202 [
> > > > 136.927835] RAX:  RBX:  RCX:
> > > > 9af2c3e353c0
> > > > [  136.927839] RDX:  RSI: 9af2c0b838e0 RDI:
> > > > 9af2c3e353c0
> > > > [  136.927843] RBP: 0001 R08:  R09:
> > > > 9af2c0b83880
> > > > [  136.927846] R10: 9af2c3e353c0 R11: 9af2c3e357c0 R12:
> > > > 03a0
> > > > [  136.927849] R13: 00025a0a R14:  R15:
> > > > 
> > > > [  136.927854] FS:  7f1eca167700()
> > > > GS:8c19fab0()
> > > > knlGS:
> > > > 
> > > > [  136.927858] CS:  0010 DS:  ES:  CR0: 80050033 [
> > > > 136.927862] CR2: 7f1ec776c000 CR3: 0001312a4003 CR4:
> > > > 003606e0
> > > > [  136.927865] Call Trace:
> > > > [  136.927884]  ? __accumulate_pelt_segments+0x29/0x3a
> > > > [  136.927892]  ? __switch_to_asm+0x40/0x70 [  136.927899]  ?
> > > > alloc_vmap_area+0x78/0x2f6 [  136.927903]  ?
> > > > __switch_to_asm+0x40/0x70 [  136.927907]  ?
> > > > __switch_to_asm+0x34/0x70 [  136.927911]  ?
> > > > __switch_to_asm+0x40/0x70 [  136.927915]  ?
> > > > __switch_to_asm+0x34/0x70 [  136.927923]  ?
> > > > __inc_numa_state+0x28/0x70 [  136.927929]  ?
> > > > preempt_latency_start+0x1e/0x3d [  136.927936]  ?
> > > > get_page_from_freelist+0x821/0xb62
> > > > [  136.927943]  ? slab_pre_alloc_hook+0x12/0x3b [  136.927948]  ?
> > > > kmem_cache_alloc_node_trace+0xf6/0x108
> > > > [  136.927954]  ? alloc_vmap_area+0x78/0x2f6
> > >
> > > Is it just me or the backtrace above doesn't seem to make sense? I
> > > don't see any allocations inside ipu3_css_cfg_acc().
> >
> > I suppose that's why it's prefixed with '?' :-)
> >
> > > > [  136.927965]  ipu3_css_cfg_acc+0xa0/0x1b5f [ipu3_imgu] [
> > > > 136.927981]  ipu3_css_set_parameters+0x286/0x6e7 [ipu3_imgu] [
> > > > 136.927995]  ipu3_css_start_streaming+0x1230/0x130a [ipu3_imgu] [
> > > > 136.928010]  imgu_s_stream+0x104/0x2f7 [ipu3_imgu] [  136.928022]
> > > > ipu3_vb2_start_streaming+0x168/0x1bd [ipu3_imgu] [  136.928034]
> > > > vb2_start_streaming+0x6c/0xf2 [videobuf2_common] [  136.928044]
> > > > vb2_core_streamon+0xcf/0x109 [videobuf2_common] [  136.928061]
> > > > __video_do_ioctl+0x239/0x388 [videodev] [  136.928081]
> > > > video_usercopy+0x25d/0x47a [videodev] [  136.928097]  ?
> > > > copy_overflow+0x14/0x14 [videodev] [  136.928115]
> > > > v4l2_ioctl+0x4d/0x58 [videodev] [  136.928123]
> > > > vfs_ioctl+0x1b/0x28 [  136.928130]  do_vfs_ioctl+0x4de/0x566 [
> > > > 136.928139]
> > > > ksys_ioctl+0x50/0x70 [  136.928146]  __x64_sys_ioctl+0x16/0x19 [
> > > > 136.928152]  do_sys

Re: [PATCH v7 00/16] Intel IPU3 ImgU patchset

2018-12-03 Thread Laurent Pinchart
Hi Sakari,

On Monday, 3 December 2018 11:51:01 EET Sakari Ailus wrote:
> On Fri, Nov 30, 2018 at 01:07:53AM +0200, Laurent Pinchart wrote:
> > On Wednesday, 7 November 2018 06:16:47 EET Bing Bu Cao wrote:
> >> On 11/01/2018 08:03 PM, Sakari Ailus wrote:
> >>> On Mon, Oct 29, 2018 at 03:22:54PM -0700, Yong Zhi wrote:
> > 
> > [snip]
> > 
>  ImgU media topology print:
>  
>  # media-ctl -d /dev/media0 -p
>  Media controller API version 4.19.0
>  
>  Media device information
>  
>  driver  ipu3-imgu
>  model   ipu3-imgu
>  serial
>  bus infoPCI::00:05.0
>  hw revision 0x80862015
>  driver version  4.19.0
>  
>  Device topology
>  - entity 1: ipu3-imgu 0 (5 pads, 5 links)
>  type V4L2 subdev subtype Unknown flags 0
>  device node name /dev/v4l-subdev0
>   pad0: Sink
>   [fmt:UYVY8_2X8/1920x1080 field:none colorspace:unknown
> >>> 
> >>> This doesn't seem right. Which formats can be enumerated from the pad?
> >>> 
>    crop:(0,0)/1920x1080
>    compose:(0,0)/1920x1080]
> >>> 
> >>> Does the compose rectangle affect the scaling on all outputs?
> >> 
> >> Sakari, driver use crop and compose targets to help set input-feeder and
> >> BDS output resolutions which are 2 key block of whole imaging pipeline,
> >> not the actual ending output, but they will impact the final output.
> >> 
>   <- "ipu3-imgu 0 input":0 []
> >>> 
> >>> Are there links that have no useful link configuration? If so, you
> >>> should set them enabled and immutable in the driver.
> >> 
> >> The enabled status of input pads is used to get which pipe that user is
> >> trying to enable (ipu3_link_setup()), so it could not been set as
> >> immutable.
> > 
> > Each pipe needs an input in order to operate, so from that point of view
> > the input is mandatory. Why can't we make this link immutable, and use
> > the stream state (VIDIOC_STREAMON/VIDIOC_STREAMOFF) to enable/disable the
> > pipes ?
> 
> There are only two options (AFAIK) in choosing the firmware, and by
> configuring the links this is better visible to the user: the links the
> state of which can be changed are not immutable. The driver can also obtain
> the explicit pipeline configuration, which makes the implementation more
> simple.

Do you mean that different firmwares are loaded based on link configuration ? 
Does it also mean that once we start using the first pipeline the 
configuration of the second pipeline can't be changed anymore ? If so, what's 
the reason for such a limitation ?

-- 
Regards,

Laurent Pinchart





Re: [PATCH v7 00/16] Intel IPU3 ImgU patchset

2018-12-03 Thread Sakari Ailus
Hi Laurent,

On Fri, Nov 30, 2018 at 01:07:53AM +0200, Laurent Pinchart wrote:
> Hello Bing,
> 
> On Wednesday, 7 November 2018 06:16:47 EET Bing Bu Cao wrote:
> > On 11/01/2018 08:03 PM, Sakari Ailus wrote:
> > > On Mon, Oct 29, 2018 at 03:22:54PM -0700, Yong Zhi wrote:
> 
> [snip]
> 
> > >> ImgU media topology print:
> > >> 
> > >> # media-ctl -d /dev/media0 -p
> > >> Media controller API version 4.19.0
> > >> 
> > >> Media device information
> > >> 
> > >> driver  ipu3-imgu
> > >> model   ipu3-imgu
> > >> serial
> > >> bus infoPCI::00:05.0
> > >> hw revision 0x80862015
> > >> driver version  4.19.0
> > >> 
> > >> Device topology
> > >> - entity 1: ipu3-imgu 0 (5 pads, 5 links)
> > >> type V4L2 subdev subtype Unknown flags 0
> > >> device node name /dev/v4l-subdev0
> > >>  pad0: Sink
> > >>  [fmt:UYVY8_2X8/1920x1080 field:none colorspace:unknown
> > > 
> > > This doesn't seem right. Which formats can be enumerated from the pad?
> > > 
> > >>   crop:(0,0)/1920x1080
> > >>   compose:(0,0)/1920x1080]
> > > 
> > > Does the compose rectangle affect the scaling on all outputs?
> > 
> > Sakari, driver use crop and compose targets to help set input-feeder and BDS
> > output resolutions which are 2 key block of whole imaging pipeline, not the
> > actual ending output, but they will impact the final output.
> > 
> > >>  <- "ipu3-imgu 0 input":0 []
> > > 
> > > Are there links that have no useful link configuration? If so, you should
> > > set them enabled and immutable in the driver.
> > 
> > The enabled status of input pads is used to get which pipe that user is
> > trying to enable (ipu3_link_setup()), so it could not been set as immutable.
> 
> Each pipe needs an input in order to operate, so from that point of view the 
> input is mandatory. Why can't we make this link immutable, and use the stream 
> state (VIDIOC_STREAMON/VIDIOC_STREAMOFF) to enable/disable the pipes ?

There are only two options (AFAIK) in choosing the firmware, and by
configuring the links this is better visible to the user: the links the
state of which can be changed are not immutable. The driver can also obtain
the explicit pipeline configuration, which makes the implementation more
simple.

-- 
Regards,

Sakari Ailus
sakari.ai...@linux.intel.com


Re: [PATCH v7 00/16] Intel IPU3 ImgU patchset

2018-11-30 Thread Sakari Ailus
Hi Laurent,

On Fri, Nov 30, 2018 at 01:09:37AM +0200, Laurent Pinchart wrote:
> Hi Sakari,
> 
> 
> On Friday, 9 November 2018 12:09:54 EET Sakari Ailus wrote:
> > On Wed, Nov 07, 2018 at 12:16:47PM +0800, Bing Bu Cao wrote:
> > > On 11/01/2018 08:03 PM, Sakari Ailus wrote:
> > >> On Mon, Oct 29, 2018 at 03:22:54PM -0700, Yong Zhi wrote:
> 
> [snip]
> 
> > >>> ImgU media topology print:
> > >>> 
> > >>> # media-ctl -d /dev/media0 -p
> > >>> Media controller API version 4.19.0
> > >>> 
> > >>> Media device information
> > >>> 
> > >>> driver  ipu3-imgu
> > >>> model   ipu3-imgu
> > >>> serial
> > >>> bus infoPCI::00:05.0
> > >>> hw revision 0x80862015
> > >>> driver version  4.19.0
> > >>> 
> > >>> Device topology
> > >>> - entity 1: ipu3-imgu 0 (5 pads, 5 links)
> > >>> type V4L2 subdev subtype Unknown flags 0
> > >>> device node name /dev/v4l-subdev0
> > >>> pad0: Sink
> > >>> [fmt:UYVY8_2X8/1920x1080 field:none colorspace:unknown
> > >> 
> > >> This doesn't seem right. Which formats can be enumerated from the pad?
> > 
> > Looking at the code, the OUTPUT video nodes have 10-bit GRBG (or a variant)
> > format whereas the CAPTURE video nodes always have NV12. Can you confirm?
> > 
> > If the OUTPUT video node format selection has no effect on the rest of the
> > pipeline (device capabilities, which processing blocks are in use, CAPTURE
> > video nodes formats etc.), I think you could simply use the FIXED media bus
> > code for each pad. That would actually make sense: this device always works
> > from memory to memory, and thus does not really have a pixel data bus
> > external to the device which is what the media bus codes really are for.
> 
> Isn't the Bayer variant useful information to configure debayering ? I would 
> expect it to be passed through the format on pad 0.

That's already configured on the video node. The FIXED media bus code is
intended for links where there's nothing to configure --- which is the case
here.

-- 
Regards,

Sakari Ailus
sakari.ai...@linux.intel.com


Re: [PATCH v7 00/16] Intel IPU3 ImgU patchset

2018-11-29 Thread Laurent Pinchart
Hi Sakari,


On Friday, 9 November 2018 12:09:54 EET Sakari Ailus wrote:
> On Wed, Nov 07, 2018 at 12:16:47PM +0800, Bing Bu Cao wrote:
> > On 11/01/2018 08:03 PM, Sakari Ailus wrote:
> >> On Mon, Oct 29, 2018 at 03:22:54PM -0700, Yong Zhi wrote:

[snip]

> >>> ImgU media topology print:
> >>> 
> >>> # media-ctl -d /dev/media0 -p
> >>> Media controller API version 4.19.0
> >>> 
> >>> Media device information
> >>> 
> >>> driver  ipu3-imgu
> >>> model   ipu3-imgu
> >>> serial
> >>> bus infoPCI::00:05.0
> >>> hw revision 0x80862015
> >>> driver version  4.19.0
> >>> 
> >>> Device topology
> >>> - entity 1: ipu3-imgu 0 (5 pads, 5 links)
> >>> type V4L2 subdev subtype Unknown flags 0
> >>> device node name /dev/v4l-subdev0
> >>> pad0: Sink
> >>> [fmt:UYVY8_2X8/1920x1080 field:none colorspace:unknown
> >> 
> >> This doesn't seem right. Which formats can be enumerated from the pad?
> 
> Looking at the code, the OUTPUT video nodes have 10-bit GRBG (or a variant)
> format whereas the CAPTURE video nodes always have NV12. Can you confirm?
> 
> If the OUTPUT video node format selection has no effect on the rest of the
> pipeline (device capabilities, which processing blocks are in use, CAPTURE
> video nodes formats etc.), I think you could simply use the FIXED media bus
> code for each pad. That would actually make sense: this device always works
> from memory to memory, and thus does not really have a pixel data bus
> external to the device which is what the media bus codes really are for.

Isn't the Bayer variant useful information to configure debayering ? I would 
expect it to be passed through the format on pad 0.

-- 
Regards,

Laurent Pinchart





Re: [PATCH v7 00/16] Intel IPU3 ImgU patchset

2018-11-29 Thread Laurent Pinchart
Hello Bing,

On Wednesday, 7 November 2018 06:16:47 EET Bing Bu Cao wrote:
> On 11/01/2018 08:03 PM, Sakari Ailus wrote:
> > On Mon, Oct 29, 2018 at 03:22:54PM -0700, Yong Zhi wrote:

[snip]

> >> ImgU media topology print:
> >> 
> >> # media-ctl -d /dev/media0 -p
> >> Media controller API version 4.19.0
> >> 
> >> Media device information
> >> 
> >> driver  ipu3-imgu
> >> model   ipu3-imgu
> >> serial
> >> bus infoPCI::00:05.0
> >> hw revision 0x80862015
> >> driver version  4.19.0
> >> 
> >> Device topology
> >> - entity 1: ipu3-imgu 0 (5 pads, 5 links)
> >> type V4L2 subdev subtype Unknown flags 0
> >> device node name /dev/v4l-subdev0
> >>pad0: Sink
> >>[fmt:UYVY8_2X8/1920x1080 field:none colorspace:unknown
> > 
> > This doesn't seem right. Which formats can be enumerated from the pad?
> > 
> >> crop:(0,0)/1920x1080
> >> compose:(0,0)/1920x1080]
> > 
> > Does the compose rectangle affect the scaling on all outputs?
> 
> Sakari, driver use crop and compose targets to help set input-feeder and BDS
> output resolutions which are 2 key block of whole imaging pipeline, not the
> actual ending output, but they will impact the final output.
> 
> >><- "ipu3-imgu 0 input":0 []
> > 
> > Are there links that have no useful link configuration? If so, you should
> > set them enabled and immutable in the driver.
> 
> The enabled status of input pads is used to get which pipe that user is
> trying to enable (ipu3_link_setup()), so it could not been set as immutable.

Each pipe needs an input in order to operate, so from that point of view the 
input is mandatory. Why can't we make this link immutable, and use the stream 
state (VIDIOC_STREAMON/VIDIOC_STREAMOFF) to enable/disable the pipes ?

> >>pad1: Sink
> >>[fmt:UYVY8_2X8/1920x1080 field:none colorspace:unknown]
> > 
> > I'd suggest to use MEDIA_BUS_FMT_FIXED here.
> > 
> >><- "ipu3-imgu 0 parameters":0 []
> >>
> >>pad2: Source
> >>[fmt:UYVY8_2X8/1920x1080 field:none colorspace:unknown]
> >>-> "ipu3-imgu 0 output":0 []
> >>
> >>pad3: Source
> >>[fmt:UYVY8_2X8/1920x1080 field:none colorspace:unknown]
> >>-> "ipu3-imgu 0 viewfinder":0 []
> > 
> > Are there other differences between output and viewfinder?
> 
> output and viewfinder are the main and secondary output of output system.
> 'main' output is not allowed to be scaled, only support crop. secondary
> output 'viewfinder' can support both cropping and scaling. User can select
> different nodes to use as preview and capture flexibly based on the actual
> use cases.

This is very useful information, thank you. Could it be added to the 
documentation (patch 02/16) with a block diagram ?

> >>pad4: Source
> >>[fmt:UYVY8_2X8/1920x1080 field:none colorspace:unknown]
> >>-> "ipu3-imgu 0 3a stat":0 []
> > 
> > FIXED here, too.
> > 
> >> - entity 7: ipu3-imgu 1 (5 pads, 5 links)
> >> type V4L2 subdev subtype Unknown flags 0
> >> device node name /dev/v4l-subdev1
> >>
> >>pad0: Sink
> >>[fmt:UYVY8_2X8/1920x1080 field:none colorspace:unknown
> >> crop:(0,0)/1920x1080
> >> compose:(0,0)/1920x1080]
> >><- "ipu3-imgu 1 input":0 []
> >>
> >>pad1: Sink
> >>[fmt:UYVY8_2X8/1920x1080 field:none colorspace:unknown]
> >><- "ipu3-imgu 1 parameters":0 []
> >>
> >>pad2: Source
> >>[fmt:UYVY8_2X8/1920x1080 field:none colorspace:unknown]
> >>-> "ipu3-imgu 1 output":0 []
> >>
> >>pad3: Source
> >>[fmt:UYVY8_2X8/1920x1080 field:none colorspace:unknown]
> >>-> "ipu3-imgu 1 viewfinder":0 []
> >>
> >>pad4: Source
> >>[fmt:UYVY8_2X8/1920x1080 field:none colorspace:unknown]
> >>-> "ipu3-imgu 1 3a stat":0 []
> > 
> > This is a minor matter but --- could you create the second sub-device
> > after the video device nodes related to the first one have been already
> > created? That'd make reading the output easier.
> > 
> >> - entity 17: ipu3-imgu 0 input (1 pad, 1 link)
> >> 
> >>  type Node subtype V4L flags 0
> >>  device node name /dev/video0
> >>
> >>pad0: Source
> >>-> "ipu3-imgu 0":0 []
> >> 
> >> - entity 23: ipu3-imgu 0 parameters (1 pad, 1 link)
> >>  type Node subtype V4L flags 0
> >>  device node name /dev/video1
> >>
> >>pad0: Source
> >>-> "ipu3-imgu 0":1 []
> >> 
> >> - entity 29: ipu3-imgu 0 output (1 pad, 1 link)
> >>  type Node subtype V4L flags 0
> >>  device node name /dev/video2
> >>
> >>pad0: Sink
> >><- "ipu3-imgu 0":2 []
> >> 
> >> - entity 35: ipu3-imgu 0 viewfinder (1 pad, 1 link)
> >>  type Node subtype V4L flags 0
> >>   

RE: [PATCH v7 00/16] Intel IPU3 ImgU patchset

2018-11-29 Thread Mani, Rajmohan
Hi Laurent, Tomasz,

Thanks for the reviews.

> Subject: Re: [PATCH v7 00/16] Intel IPU3 ImgU patchset
> 
> Hi Tomasz,
> 
> On Thursday, 29 November 2018 21:51:32 EET Tomasz Figa wrote:
> > On Thu, Nov 29, 2018 at 6:43 AM Laurent Pinchart wrote:
> > > On Tuesday, 30 October 2018 00:22:54 EET Yong Zhi wrote:
> 
> [snip]
> 
> > >> 1. Link pad flag of video nodes (i.e. ipu3-imgu 0 output) need to
> > >> be enabled prior to the test.
> > >> 2. Stream tests are not performed since it requires
> > >> pre-configuration for each case.
> > >
> > > And that's a bit of an issue. I've tested the driver with a small
> > > script based on media-ctl to configure links and yavta to interface
> > > with the video nodes, and got the following oops:
> > >
> > > [  136.927788] divide error:  [#1] PREEMPT SMP PTI [
> > > 136.927801] CPU: 2 PID: 2069 Comm: yavta Not tainted 4.20.0-rc1+ #9
> > > [  136.927806] Hardware name: HP Soraka/Soraka, BIOS  08/30/2018 [
> > > 136.927820] RIP: 0010:ipu3_css_osys_calc+0xc54/0xe14 [ipu3_imgu] [
> > > 136.927825] Code: 89 44 24 28 42 8b 44 86 6c f7 54 24 04 81 64 24 28
> > > 00 fd ff ff 81 64 24 04 00 03 00 00 8d 44 03 ff 81 44 24 28 80 03 00
> > > 00 99  fb 0f af c3 bb 20 00 00 00 99 f7 fb 8b 5c 24 40 83 fd 01
> > > 19 d2 [  136.927830] RSP: 0018:9af2c0b837c8 EFLAGS: 00010202 [
> > > 136.927835] RAX:  RBX:  RCX:
> > > 9af2c3e353c0
> > > [  136.927839] RDX:  RSI: 9af2c0b838e0 RDI:
> > > 9af2c3e353c0
> > > [  136.927843] RBP: 0001 R08:  R09:
> > > 9af2c0b83880
> > > [  136.927846] R10: 9af2c3e353c0 R11: 9af2c3e357c0 R12:
> > > 03a0
> > > [  136.927849] R13: 00025a0a R14:  R15:
> > > 
> > > [  136.927854] FS:  7f1eca167700() GS:8c19fab0()
> > > knlGS:
> > > 
> > > [  136.927858] CS:  0010 DS:  ES:  CR0: 80050033 [
> > > 136.927862] CR2: 7f1ec776c000 CR3: 0001312a4003 CR4:
> > > 003606e0
> > > [  136.927865] Call Trace:
> > > [  136.927884]  ? __accumulate_pelt_segments+0x29/0x3a
> > > [  136.927892]  ? __switch_to_asm+0x40/0x70 [  136.927899]  ?
> > > alloc_vmap_area+0x78/0x2f6 [  136.927903]  ?
> > > __switch_to_asm+0x40/0x70 [  136.927907]  ?
> > > __switch_to_asm+0x34/0x70 [  136.927911]  ?
> > > __switch_to_asm+0x40/0x70 [  136.927915]  ?
> > > __switch_to_asm+0x34/0x70 [  136.927923]  ?
> > > __inc_numa_state+0x28/0x70 [  136.927929]  ?
> > > preempt_latency_start+0x1e/0x3d [  136.927936]  ?
> > > get_page_from_freelist+0x821/0xb62
> > > [  136.927943]  ? slab_pre_alloc_hook+0x12/0x3b [  136.927948]  ?
> > > kmem_cache_alloc_node_trace+0xf6/0x108
> > > [  136.927954]  ? alloc_vmap_area+0x78/0x2f6
> >
> > Is it just me or the backtrace above doesn't seem to make sense? I
> > don't see any allocations inside ipu3_css_cfg_acc().
> 
> I suppose that's why it's prefixed with '?' :-)
> 
> > > [  136.927965]  ipu3_css_cfg_acc+0xa0/0x1b5f [ipu3_imgu] [
> > > 136.927981]  ipu3_css_set_parameters+0x286/0x6e7 [ipu3_imgu] [
> > > 136.927995]  ipu3_css_start_streaming+0x1230/0x130a [ipu3_imgu] [
> > > 136.928010]  imgu_s_stream+0x104/0x2f7 [ipu3_imgu] [  136.928022]
> > > ipu3_vb2_start_streaming+0x168/0x1bd [ipu3_imgu] [  136.928034]
> > > vb2_start_streaming+0x6c/0xf2 [videobuf2_common] [  136.928044]
> > > vb2_core_streamon+0xcf/0x109 [videobuf2_common] [  136.928061]
> > > __video_do_ioctl+0x239/0x388 [videodev] [  136.928081]
> > > video_usercopy+0x25d/0x47a [videodev] [  136.928097]  ?
> > > copy_overflow+0x14/0x14 [videodev] [  136.928115]
> > > v4l2_ioctl+0x4d/0x58 [videodev] [  136.928123]  vfs_ioctl+0x1b/0x28
> > > [  136.928130]  do_vfs_ioctl+0x4de/0x566 [  136.928139]
> > > ksys_ioctl+0x50/0x70 [  136.928146]  __x64_sys_ioctl+0x16/0x19 [
> > > 136.928152]  do_syscall_64+0x4d/0x5a [  136.928158]
> > > entry_SYSCALL_64_after_hwframe+0x44/0xa9
> > > [  136.928164] RIP: 0033:0x7f1ec9a84f47 [  136.928169] Code: 00 00
> > > 00 48 8b 05 51 6f 2c 00 64 c7 00 26 00 00 00 48
> > > c7 c0 ff ff ff ff c3 66 2e 0f 1f 84 00 00 00 00 00 b8 10 00 00 00 0f
> > > 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 21 6f 2c 00 f7 d8 64 89 01
> > > 48 [  136.928173] RSP: 002b:7ffe279e6188 EFLAGS: 0246
> ORIG_RA

Re: [PATCH v7 00/16] Intel IPU3 ImgU patchset

2018-11-29 Thread Laurent Pinchart
Hi Tomasz,

On Thursday, 29 November 2018 21:51:32 EET Tomasz Figa wrote:
> On Thu, Nov 29, 2018 at 6:43 AM Laurent Pinchart wrote:
> > On Tuesday, 30 October 2018 00:22:54 EET Yong Zhi wrote:

[snip]

> >> 1. Link pad flag of video nodes (i.e. ipu3-imgu 0 output) need to be
> >> enabled prior to the test.
> >> 2. Stream tests are not performed since it requires pre-configuration
> >> for each case.
> > 
> > And that's a bit of an issue. I've tested the driver with a small script
> > based on media-ctl to configure links and yavta to interface with the
> > video nodes, and got the following oops:
> > 
> > [  136.927788] divide error:  [#1] PREEMPT SMP PTI
> > [  136.927801] CPU: 2 PID: 2069 Comm: yavta Not tainted 4.20.0-rc1+ #9
> > [  136.927806] Hardware name: HP Soraka/Soraka, BIOS  08/30/2018
> > [  136.927820] RIP: 0010:ipu3_css_osys_calc+0xc54/0xe14 [ipu3_imgu]
> > [  136.927825] Code: 89 44 24 28 42 8b 44 86 6c f7 54 24 04 81 64 24 28 00
> > fd ff ff 81 64 24 04 00 03 00 00 8d 44 03 ff 81 44 24 28 80 03 00 00 99
> >  fb 0f af c3 bb 20 00 00 00 99 f7 fb 8b 5c 24 40 83 fd 01 19 d2
> > [  136.927830] RSP: 0018:9af2c0b837c8 EFLAGS: 00010202
> > [  136.927835] RAX:  RBX:  RCX:
> > 9af2c3e353c0
> > [  136.927839] RDX:  RSI: 9af2c0b838e0 RDI:
> > 9af2c3e353c0
> > [  136.927843] RBP: 0001 R08:  R09:
> > 9af2c0b83880
> > [  136.927846] R10: 9af2c3e353c0 R11: 9af2c3e357c0 R12:
> > 03a0
> > [  136.927849] R13: 00025a0a R14:  R15:
> > 
> > [  136.927854] FS:  7f1eca167700() GS:8c19fab0()
> > knlGS:
> > 
> > [  136.927858] CS:  0010 DS:  ES:  CR0: 80050033
> > [  136.927862] CR2: 7f1ec776c000 CR3: 0001312a4003 CR4:
> > 003606e0
> > [  136.927865] Call Trace:
> > [  136.927884]  ? __accumulate_pelt_segments+0x29/0x3a
> > [  136.927892]  ? __switch_to_asm+0x40/0x70
> > [  136.927899]  ? alloc_vmap_area+0x78/0x2f6
> > [  136.927903]  ? __switch_to_asm+0x40/0x70
> > [  136.927907]  ? __switch_to_asm+0x34/0x70
> > [  136.927911]  ? __switch_to_asm+0x40/0x70
> > [  136.927915]  ? __switch_to_asm+0x34/0x70
> > [  136.927923]  ? __inc_numa_state+0x28/0x70
> > [  136.927929]  ? preempt_latency_start+0x1e/0x3d
> > [  136.927936]  ? get_page_from_freelist+0x821/0xb62
> > [  136.927943]  ? slab_pre_alloc_hook+0x12/0x3b
> > [  136.927948]  ? kmem_cache_alloc_node_trace+0xf6/0x108
> > [  136.927954]  ? alloc_vmap_area+0x78/0x2f6
> 
> Is it just me or the backtrace above doesn't seem to make sense? I
> don't see any allocations inside ipu3_css_cfg_acc().

I suppose that's why it's prefixed with '?' :-)

> > [  136.927965]  ipu3_css_cfg_acc+0xa0/0x1b5f [ipu3_imgu]
> > [  136.927981]  ipu3_css_set_parameters+0x286/0x6e7 [ipu3_imgu]
> > [  136.927995]  ipu3_css_start_streaming+0x1230/0x130a [ipu3_imgu]
> > [  136.928010]  imgu_s_stream+0x104/0x2f7 [ipu3_imgu]
> > [  136.928022]  ipu3_vb2_start_streaming+0x168/0x1bd [ipu3_imgu]
> > [  136.928034]  vb2_start_streaming+0x6c/0xf2 [videobuf2_common]
> > [  136.928044]  vb2_core_streamon+0xcf/0x109 [videobuf2_common]
> > [  136.928061]  __video_do_ioctl+0x239/0x388 [videodev]
> > [  136.928081]  video_usercopy+0x25d/0x47a [videodev]
> > [  136.928097]  ? copy_overflow+0x14/0x14 [videodev]
> > [  136.928115]  v4l2_ioctl+0x4d/0x58 [videodev]
> > [  136.928123]  vfs_ioctl+0x1b/0x28
> > [  136.928130]  do_vfs_ioctl+0x4de/0x566
> > [  136.928139]  ksys_ioctl+0x50/0x70
> > [  136.928146]  __x64_sys_ioctl+0x16/0x19
> > [  136.928152]  do_syscall_64+0x4d/0x5a
> > [  136.928158]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
> > [  136.928164] RIP: 0033:0x7f1ec9a84f47
> > [  136.928169] Code: 00 00 00 48 8b 05 51 6f 2c 00 64 c7 00 26 00 00 00 48
> > c7 c0 ff ff ff ff c3 66 2e 0f 1f 84 00 00 00 00 00 b8 10 00 00 00 0f 05
> > <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 21 6f 2c 00 f7 d8 64 89 01 48
> > [  136.928173] RSP: 002b:7ffe279e6188 EFLAGS: 0246 ORIG_RAX:
> > 0010
> > [  136.928178] RAX: ffda RBX: 0007 RCX:
> > 7f1ec9a84f47
> > [  136.928181] RDX: 7ffe279e6194 RSI: 40045612 RDI:
> > 0003
> > [  136.928184] RBP:  R08: 7f1ec776d000 R09:
> > 
> > [  136.928188] R10: 0020 R11: 0246 R12:
> > 7ffe279e6360
> > [  136.928191] R13: 0004 R14: 7ffe279e6360 R15:
> > 7ffe279e8826
> > [  136.928198] Modules linked in: ccm zram arc4 iwlmvm mac80211 intel_rapl
> > x86_pkg_temp_thermal intel_powerclamp coretemp iwlwifi cfg80211
> > hid_multitouch ipu3_imgu ipu3_cio2 8250_dw videobuf2_dma_sg
> > videobuf2_memops videobuf2_v4l2 processor_thermal_device
> > intel_soc_dts_iosf videobuf2_common ov5670 ov13858 dw9714 v4l2_fwnode
> > v4l2_common videodev media at24 cros_ec_lpcs cros_ec_core int3403_thermal
> > int340x_thermal_zone int3400_thermal 

Re: [PATCH v7 00/16] Intel IPU3 ImgU patchset

2018-11-29 Thread Tomasz Figa
On Thu, Nov 29, 2018 at 6:43 AM Laurent Pinchart
 wrote:
>
> Hello Yong,
>
> On Tuesday, 30 October 2018 00:22:54 EET Yong Zhi wrote:
> > Hi,
> >
> > This series adds support for the Intel IPU3 (Image Processing Unit)
> > ImgU which is essentially a modern memory-to-memory ISP. It implements
> > raw Bayer to YUV image format conversion as well as a large number of
> > other pixel processing algorithms for improving the image quality.
> >
> > Meta data formats are defined for image statistics (3A, i.e. automatic
> > white balance, exposure and focus, histogram and local area contrast
> > enhancement) as well as for the pixel processing algorithm parameters.
> > The documentation for these formats is currently not included in the
> > patchset but will be added in a future version of this set.
> >
> > The algorithm parameters need to be considered specific to a given frame
> > and typically a large number of these parameters change on frame to frame
> > basis. Additionally, the parameters are highly structured (and not a flat
> > space of independent configuration primitives). They also reflect the
> > data structures used by the firmware and the hardware. On top of that,
> > the algorithms require highly specialized user space to make meaningful
> > use of them. For these reasons it has been chosen video buffers to pass
> > the parameters to the device.
> >
> > On individual patches:
> >
> > The heart of ImgU is the CSS, or Camera Subsystem, which contains the
> > image processors and HW accelerators.
> >
> > The 3A statistics and other firmware parameter computation related
> > functions are implemented in patch 11.
> >
> > All IPU3 pipeline default settings can be found in patch 10.
> >
> > To access DDR via ImgU's own memory space, IPU3 is also equipped with
> > its own MMU unit, the driver is implemented in patch 6.
> >
> > Patch 7 uses above driver for DMA mapping operation.
> >
> > The communication between IPU3 firmware and driver is implemented with
> > circular queues in patch 8.
> >
> > Patch 9 provide some utility functions and manage IPU3 fw download and
> > install.
> >
> > The firmware which is called ipu3-fw.bin can be downloaded from:
> >
> > git://git.kernel.org/pub/scm/linux/kernel/git/firmware/linux-firmware.git
> > (commit 2c27b0cb02f18c022d8378e0e1abaf8b7ae8188f)
> >
> > Firmware ABI is defined in patches 4 and 5.
> >
> > Patches 12 and 13 are of the same file, the former contains all h/w
> > programming related code, the latter implements interface functions for
> > access fw & hw capabilities.
> >
> > Patch 14 has a dependency on Sakari's V4L2_BUF_TYPE_META_OUTPUT work:
> >
> > https://patchwork.kernel.org/patch/9976295/>
> >
> > Patch 15 represents the top level that glues all of the other components
> > together, passing arguments between the components.
> >
> > Patch 16 is a recent effort to extend v6 for advanced camera features like
> > Continuous View Finder (CVF) and Snapshot During Video(SDV) support.
> >
> > Link to user space implementation:
> >
> > git clone https://chromium.googlesource.com/chromiumos/platform/arc-camera
> >
> > ImgU media topology print:
>
> [snip]
>
> > v4l2-compliance utility is built with Sakari's patches for meta data
> > output support(rebased):
> >
> > https://patchwork.linuxtv.org/patch/43370/>
> > https://patchwork.linuxtv.org/patch/43369/>
> >
> > The test (v4l2-compliance -m 0) passes without error, outputs are appended
> > at the end of revision history.
> >
> > Note:
> >
> > 1. Link pad flag of video nodes (i.e. ipu3-imgu 0 output) need to be enabled
> > prior to the test.
> > 2. Stream tests are not performed since it requires pre-configuration for
> > each case.
>
> And that's a bit of an issue. I've tested the driver with a small script based
> on media-ctl to configure links and yavta to interface with the video nodes,
> and got the following oops:
>
> [  136.927788] divide error:  [#1] PREEMPT SMP PTI
> [  136.927801] CPU: 2 PID: 2069 Comm: yavta Not tainted 4.20.0-rc1+ #9
> [  136.927806] Hardware name: HP Soraka/Soraka, BIOS  08/30/2018
> [  136.927820] RIP: 0010:ipu3_css_osys_calc+0xc54/0xe14 [ipu3_imgu]
> [  136.927825] Code: 89 44 24 28 42 8b 44 86 6c f7 54 24 04 81 64 24 28 00 fd
> ff ff 81 64 24 04 00 03 00 00 8d 44 03 ff 81 44 24 28 80 03 00 00 99  fb
> 0f af c3 bb 20 00 00 00 99 f7 fb 8b 5c 24 40 83 fd 01 19 d2
> [  136.927830] RSP: 0018:9af2c0b837c8 EFLAGS: 00010202
> [  136.927835] RAX:  RBX:  RCX:
> 9af2c3e353c0
> [  136.927839] RDX:  RSI: 9af2c0b838e0 RDI:
> 9af2c3e353c0
> [  136.927843] RBP: 0001 R08:  R09:
> 9af2c0b83880
> [  136.927846] R10: 9af2c3e353c0 R11: 9af2c3e357c0 R12:
> 03a0
> [  136.927849] R13: 00025a0a R14:  R15:
> 
> [  136.927854] FS:  7f1eca167700() GS:8c19fab0() knlGS:
> 
> [  136.927858] CS:  0010 DS:  ES: 

Re: [PATCH v7 00/16] Intel IPU3 ImgU patchset

2018-11-29 Thread Laurent Pinchart
Hello Yong,

On Tuesday, 30 October 2018 00:22:54 EET Yong Zhi wrote:
> Hi,
> 
> This series adds support for the Intel IPU3 (Image Processing Unit)
> ImgU which is essentially a modern memory-to-memory ISP. It implements
> raw Bayer to YUV image format conversion as well as a large number of
> other pixel processing algorithms for improving the image quality.
> 
> Meta data formats are defined for image statistics (3A, i.e. automatic
> white balance, exposure and focus, histogram and local area contrast
> enhancement) as well as for the pixel processing algorithm parameters.
> The documentation for these formats is currently not included in the
> patchset but will be added in a future version of this set.
> 
> The algorithm parameters need to be considered specific to a given frame
> and typically a large number of these parameters change on frame to frame
> basis. Additionally, the parameters are highly structured (and not a flat
> space of independent configuration primitives). They also reflect the
> data structures used by the firmware and the hardware. On top of that,
> the algorithms require highly specialized user space to make meaningful
> use of them. For these reasons it has been chosen video buffers to pass
> the parameters to the device.
> 
> On individual patches:
> 
> The heart of ImgU is the CSS, or Camera Subsystem, which contains the
> image processors and HW accelerators.
> 
> The 3A statistics and other firmware parameter computation related
> functions are implemented in patch 11.
> 
> All IPU3 pipeline default settings can be found in patch 10.
> 
> To access DDR via ImgU's own memory space, IPU3 is also equipped with
> its own MMU unit, the driver is implemented in patch 6.
> 
> Patch 7 uses above driver for DMA mapping operation.
> 
> The communication between IPU3 firmware and driver is implemented with
> circular queues in patch 8.
> 
> Patch 9 provide some utility functions and manage IPU3 fw download and
> install.
> 
> The firmware which is called ipu3-fw.bin can be downloaded from:
> 
> git://git.kernel.org/pub/scm/linux/kernel/git/firmware/linux-firmware.git
> (commit 2c27b0cb02f18c022d8378e0e1abaf8b7ae8188f)
> 
> Firmware ABI is defined in patches 4 and 5.
> 
> Patches 12 and 13 are of the same file, the former contains all h/w
> programming related code, the latter implements interface functions for
> access fw & hw capabilities.
> 
> Patch 14 has a dependency on Sakari's V4L2_BUF_TYPE_META_OUTPUT work:
> 
> https://patchwork.kernel.org/patch/9976295/>
> 
> Patch 15 represents the top level that glues all of the other components
> together, passing arguments between the components.
> 
> Patch 16 is a recent effort to extend v6 for advanced camera features like
> Continuous View Finder (CVF) and Snapshot During Video(SDV) support.
> 
> Link to user space implementation:
> 
> git clone https://chromium.googlesource.com/chromiumos/platform/arc-camera
> 
> ImgU media topology print:

[snip]

> v4l2-compliance utility is built with Sakari's patches for meta data
> output support(rebased):
> 
> https://patchwork.linuxtv.org/patch/43370/>
> https://patchwork.linuxtv.org/patch/43369/>
> 
> The test (v4l2-compliance -m 0) passes without error, outputs are appended
> at the end of revision history.
> 
> Note:
> 
> 1. Link pad flag of video nodes (i.e. ipu3-imgu 0 output) need to be enabled
> prior to the test.
> 2. Stream tests are not performed since it requires pre-configuration for
> each case.

And that's a bit of an issue. I've tested the driver with a small script based 
on media-ctl to configure links and yavta to interface with the video nodes, 
and got the following oops:

[  136.927788] divide error:  [#1] PREEMPT SMP PTI
[  136.927801] CPU: 2 PID: 2069 Comm: yavta Not tainted 4.20.0-rc1+ #9
[  136.927806] Hardware name: HP Soraka/Soraka, BIOS  08/30/2018
[  136.927820] RIP: 0010:ipu3_css_osys_calc+0xc54/0xe14 [ipu3_imgu]
[  136.927825] Code: 89 44 24 28 42 8b 44 86 6c f7 54 24 04 81 64 24 28 00 fd 
ff ff 81 64 24 04 00 03 00 00 8d 44 03 ff 81 44 24 28 80 03 00 00 99  fb 
0f af c3 bb 20 00 00 00 99 f7 fb 8b 5c 24 40 83 fd 01 19 d2
[  136.927830] RSP: 0018:9af2c0b837c8 EFLAGS: 00010202
[  136.927835] RAX:  RBX:  RCX: 
9af2c3e353c0
[  136.927839] RDX:  RSI: 9af2c0b838e0 RDI: 
9af2c3e353c0
[  136.927843] RBP: 0001 R08:  R09: 
9af2c0b83880
[  136.927846] R10: 9af2c3e353c0 R11: 9af2c3e357c0 R12: 
03a0
[  136.927849] R13: 00025a0a R14:  R15: 

[  136.927854] FS:  7f1eca167700() GS:8c19fab0() knlGS:

[  136.927858] CS:  0010 DS:  ES:  CR0: 80050033
[  136.927862] CR2: 7f1ec776c000 CR3: 0001312a4003 CR4: 
003606e0
[  136.927865] Call Trace:
[  136.927884]  ? __accumulate_pelt_segments+0x29/0x3a
[  136.927892]  ? __switch_to_asm+0x40/0x70
[  136.927899]  ? 

Re: [PATCH v7 00/16] Intel IPU3 ImgU patchset

2018-11-17 Thread jacopo mondi
Hi Sakari,

On Wed, Nov 14, 2018 at 09:40:50AM +0200, Sakari Ailus wrote:
> Hi Jacopo,
>
> On Wed, Nov 14, 2018 at 01:25:11AM +0100, jacopo mondi wrote:
> > On Mon, Oct 29, 2018 at 03:22:54PM -0700, Yong Zhi wrote:
> > > Hi,
> > >
> > > This series adds support for the Intel IPU3 (Image Processing Unit)
> > > ImgU which is essentially a modern memory-to-memory ISP. It implements
> > > raw Bayer to YUV image format conversion as well as a large number of
> > > other pixel processing algorithms for improving the image quality.
> > >
> > > Meta data formats are defined for image statistics (3A, i.e. automatic
> > > white balance, exposure and focus, histogram and local area contrast
> > > enhancement) as well as for the pixel processing algorithm parameters.
> > > The documentation for these formats is currently not included in the
> > > patchset but will be added in a future version of this set.
> > >
> > > The algorithm parameters need to be considered specific to a given frame
> > > and typically a large number of these parameters change on frame to frame
> > > basis. Additionally, the parameters are highly structured (and not a flat
> > > space of independent configuration primitives). They also reflect the
> > > data structures used by the firmware and the hardware. On top of that,
> > > the algorithms require highly specialized user space to make meaningful
> > > use of them. For these reasons it has been chosen video buffers to pass
> > > the parameters to the device.
> > >
> > > On individual patches:
> > >
> > > The heart of ImgU is the CSS, or Camera Subsystem, which contains the
> > > image processors and HW accelerators.
> > >
> > > The 3A statistics and other firmware parameter computation related
> > > functions are implemented in patch 11.
> > >
> > > All IPU3 pipeline default settings can be found in patch 10.
> > >
> >
> > Seems to me that patch 10 didn't make it to the mailing list, am I
> > wrong?
> >
> > I'm pointing it out as the same happened on your v6.
>
> Thanks for pointing this out. I've uploaded the entire set here:
>
> https://git.linuxtv.org/sailus/media_tree.git/log/?h=ipu3-v7>
>
> including the 10th patch:
>
> https://git.linuxtv.org/sailus/media_tree.git/commit/?h=ipu3-v7=41e2f0d114dbc195efed079202d22748ddedbe83>
>
> It's too big to get through the list server. :-(
>
> Luckily, it's mostly tables so there's not much to review there. Default
> settings, effectively.

Thanks, I've now received all patches.

Thanks
   j

>
> --
> Regards,
>
> Sakari Ailus
> sakari.ai...@linux.intel.com


signature.asc
Description: PGP signature


Re: [PATCH v7 00/16] Intel IPU3 ImgU patchset

2018-11-13 Thread Sakari Ailus
Hi Jacopo,

On Wed, Nov 14, 2018 at 01:25:11AM +0100, jacopo mondi wrote:
> On Mon, Oct 29, 2018 at 03:22:54PM -0700, Yong Zhi wrote:
> > Hi,
> >
> > This series adds support for the Intel IPU3 (Image Processing Unit)
> > ImgU which is essentially a modern memory-to-memory ISP. It implements
> > raw Bayer to YUV image format conversion as well as a large number of
> > other pixel processing algorithms for improving the image quality.
> >
> > Meta data formats are defined for image statistics (3A, i.e. automatic
> > white balance, exposure and focus, histogram and local area contrast
> > enhancement) as well as for the pixel processing algorithm parameters.
> > The documentation for these formats is currently not included in the
> > patchset but will be added in a future version of this set.
> >
> > The algorithm parameters need to be considered specific to a given frame
> > and typically a large number of these parameters change on frame to frame
> > basis. Additionally, the parameters are highly structured (and not a flat
> > space of independent configuration primitives). They also reflect the
> > data structures used by the firmware and the hardware. On top of that,
> > the algorithms require highly specialized user space to make meaningful
> > use of them. For these reasons it has been chosen video buffers to pass
> > the parameters to the device.
> >
> > On individual patches:
> >
> > The heart of ImgU is the CSS, or Camera Subsystem, which contains the
> > image processors and HW accelerators.
> >
> > The 3A statistics and other firmware parameter computation related
> > functions are implemented in patch 11.
> >
> > All IPU3 pipeline default settings can be found in patch 10.
> >
> 
> Seems to me that patch 10 didn't make it to the mailing list, am I
> wrong?
> 
> I'm pointing it out as the same happened on your v6.

Thanks for pointing this out. I've uploaded the entire set here:

https://git.linuxtv.org/sailus/media_tree.git/log/?h=ipu3-v7>

including the 10th patch:

https://git.linuxtv.org/sailus/media_tree.git/commit/?h=ipu3-v7=41e2f0d114dbc195efed079202d22748ddedbe83>

It's too big to get through the list server. :-(

Luckily, it's mostly tables so there's not much to review there. Default
settings, effectively.

-- 
Regards,

Sakari Ailus
sakari.ai...@linux.intel.com


Re: [PATCH v7 00/16] Intel IPU3 ImgU patchset

2018-11-13 Thread Bing Bu Cao



On 11/14/2018 05:58 AM, Sakari Ailus wrote:
> On Tue, Nov 13, 2018 at 07:04:01PM +0800, Bing Bu Cao wrote:
>>
>> On 11/13/2018 06:31 PM, Sakari Ailus wrote:
>>> Hi Bing Bu,
>>>
>>> On Mon, Nov 12, 2018 at 12:31:16PM +0800, Bing Bu Cao wrote:
 On 11/09/2018 06:09 PM, Sakari Ailus wrote:
> Hi Bing Bu,
>
> On Wed, Nov 07, 2018 at 12:16:47PM +0800, Bing Bu Cao wrote:
>> On 11/01/2018 08:03 PM, Sakari Ailus wrote:
>>> Hi Yong,
>>>
>>> Thanks for the update!
>>>
>>> On Mon, Oct 29, 2018 at 03:22:54PM -0700, Yong Zhi wrote:
 Hi,

 This series adds support for the Intel IPU3 (Image Processing Unit)
 ImgU which is essentially a modern memory-to-memory ISP. It implements
 raw Bayer to YUV image format conversion as well as a large number of
 other pixel processing algorithms for improving the image quality.

 Meta data formats are defined for image statistics (3A, i.e. automatic
 white balance, exposure and focus, histogram and local area contrast
 enhancement) as well as for the pixel processing algorithm parameters.
 The documentation for these formats is currently not included in the
 patchset but will be added in a future version of this set.

 The algorithm parameters need to be considered specific to a given 
 frame
 and typically a large number of these parameters change on frame to 
 frame
 basis. Additionally, the parameters are highly structured (and not a 
 flat
 space of independent configuration primitives). They also reflect the
 data structures used by the firmware and the hardware. On top of that,
 the algorithms require highly specialized user space to make meaningful
 use of them. For these reasons it has been chosen video buffers to pass
 the parameters to the device.

 On individual patches:

 The heart of ImgU is the CSS, or Camera Subsystem, which contains the
 image processors and HW accelerators.

 The 3A statistics and other firmware parameter computation related
 functions are implemented in patch 11.

 All IPU3 pipeline default settings can be found in patch 10.

 To access DDR via ImgU's own memory space, IPU3 is also equipped with
 its own MMU unit, the driver is implemented in patch 6.

 Patch 7 uses above driver for DMA mapping operation.

 The communication between IPU3 firmware and driver is implemented with 
 circular
 queues in patch 8.

 Patch 9 provide some utility functions and manage IPU3 fw download and
 install.

 The firmware which is called ipu3-fw.bin can be downloaded from:

 git://git.kernel.org/pub/scm/linux/kernel/git/firmware/linux-firmware.git
 (commit 2c27b0cb02f18c022d8378e0e1abaf8b7ae8188f)

 Firmware ABI is defined in patches 4 and 5.

 Patches 12 and 13 are of the same file, the former contains all h/w 
 programming
 related code, the latter implements interface functions for access fw 
 & hw
 capabilities.

 Patch 14 has a dependency on Sakari's V4L2_BUF_TYPE_META_OUTPUT work:

 https://patchwork.kernel.org/patch/9976295/>
>>> I've pushed the latest set here:
>>>
>>> https://git.linuxtv.org/sailus/media_tree.git/log/?h=meta-output>
>>>
>>> You can just say the entire set depends on those going forward; the
>>> documentation is needed, too.
>>>
 Patch 15 represents the top level that glues all of the other 
 components together,
 passing arguments between the components.

 Patch 16 is a recent effort to extend v6 for advanced camera features 
 like
 Continuous View Finder (CVF) and Snapshot During Video(SDV) support.

 Link to user space implementation:

 git clone 
 https://chromium.googlesource.com/chromiumos/platform/arc-camera

 ImgU media topology print:

 # media-ctl -d /dev/media0 -p
 Media controller API version 4.19.0

 Media device information
 
 driver  ipu3-imgu
 model   ipu3-imgu
 serial  
 bus infoPCI::00:05.0
 hw revision 0x80862015
 driver version  4.19.0

 Device topology
 - entity 1: ipu3-imgu 0 (5 pads, 5 links)
 type V4L2 subdev subtype Unknown flags 0
 device node name /dev/v4l-subdev0
pad0: Sink
[fmt:UYVY8_2X8/1920x1080 field:none colorspace:unknown
>>> This doesn't seem right. Which formats can be 

Re: [PATCH v7 00/16] Intel IPU3 ImgU patchset

2018-11-13 Thread jacopo mondi
On Mon, Oct 29, 2018 at 03:22:54PM -0700, Yong Zhi wrote:
> Hi,
>
> This series adds support for the Intel IPU3 (Image Processing Unit)
> ImgU which is essentially a modern memory-to-memory ISP. It implements
> raw Bayer to YUV image format conversion as well as a large number of
> other pixel processing algorithms for improving the image quality.
>
> Meta data formats are defined for image statistics (3A, i.e. automatic
> white balance, exposure and focus, histogram and local area contrast
> enhancement) as well as for the pixel processing algorithm parameters.
> The documentation for these formats is currently not included in the
> patchset but will be added in a future version of this set.
>
> The algorithm parameters need to be considered specific to a given frame
> and typically a large number of these parameters change on frame to frame
> basis. Additionally, the parameters are highly structured (and not a flat
> space of independent configuration primitives). They also reflect the
> data structures used by the firmware and the hardware. On top of that,
> the algorithms require highly specialized user space to make meaningful
> use of them. For these reasons it has been chosen video buffers to pass
> the parameters to the device.
>
> On individual patches:
>
> The heart of ImgU is the CSS, or Camera Subsystem, which contains the
> image processors and HW accelerators.
>
> The 3A statistics and other firmware parameter computation related
> functions are implemented in patch 11.
>
> All IPU3 pipeline default settings can be found in patch 10.
>

Seems to me that patch 10 didn't make it to the mailing list, am I
wrong?

I'm pointing it out as the same happened on your v6.

Thanks
   j

> To access DDR via ImgU's own memory space, IPU3 is also equipped with
> its own MMU unit, the driver is implemented in patch 6.
>
> Patch 7 uses above driver for DMA mapping operation.
>
> The communication between IPU3 firmware and driver is implemented with 
> circular
> queues in patch 8.
>
> Patch 9 provide some utility functions and manage IPU3 fw download and
> install.
>
> The firmware which is called ipu3-fw.bin can be downloaded from:
>
> git://git.kernel.org/pub/scm/linux/kernel/git/firmware/linux-firmware.git
> (commit 2c27b0cb02f18c022d8378e0e1abaf8b7ae8188f)
>
> Firmware ABI is defined in patches 4 and 5.
>
> Patches 12 and 13 are of the same file, the former contains all h/w 
> programming
> related code, the latter implements interface functions for access fw & hw
> capabilities.
>
> Patch 14 has a dependency on Sakari's V4L2_BUF_TYPE_META_OUTPUT work:
>
> https://patchwork.kernel.org/patch/9976295/>
>
> Patch 15 represents the top level that glues all of the other components 
> together,
> passing arguments between the components.
>
> Patch 16 is a recent effort to extend v6 for advanced camera features like
> Continuous View Finder (CVF) and Snapshot During Video(SDV) support.
>
> Link to user space implementation:
>
> git clone https://chromium.googlesource.com/chromiumos/platform/arc-camera
>
> ImgU media topology print:
>
> # media-ctl -d /dev/media0 -p
> Media controller API version 4.19.0
>
> Media device information
> 
> driver  ipu3-imgu
> model   ipu3-imgu
> serial
> bus infoPCI::00:05.0
> hw revision 0x80862015
> driver version  4.19.0
>
> Device topology
> - entity 1: ipu3-imgu 0 (5 pads, 5 links)
> type V4L2 subdev subtype Unknown flags 0
> device node name /dev/v4l-subdev0
>   pad0: Sink
>   [fmt:UYVY8_2X8/1920x1080 field:none colorspace:unknown
>crop:(0,0)/1920x1080
>compose:(0,0)/1920x1080]
>   <- "ipu3-imgu 0 input":0 []
>   pad1: Sink
>   [fmt:UYVY8_2X8/1920x1080 field:none colorspace:unknown]
>   <- "ipu3-imgu 0 parameters":0 []
>   pad2: Source
>   [fmt:UYVY8_2X8/1920x1080 field:none colorspace:unknown]
>   -> "ipu3-imgu 0 output":0 []
>   pad3: Source
>   [fmt:UYVY8_2X8/1920x1080 field:none colorspace:unknown]
>   -> "ipu3-imgu 0 viewfinder":0 []
>   pad4: Source
>   [fmt:UYVY8_2X8/1920x1080 field:none colorspace:unknown]
>   -> "ipu3-imgu 0 3a stat":0 []
>
> - entity 7: ipu3-imgu 1 (5 pads, 5 links)
> type V4L2 subdev subtype Unknown flags 0
> device node name /dev/v4l-subdev1
>   pad0: Sink
>   [fmt:UYVY8_2X8/1920x1080 field:none colorspace:unknown
>crop:(0,0)/1920x1080
>compose:(0,0)/1920x1080]
>   <- "ipu3-imgu 1 input":0 []
>   pad1: Sink
>   [fmt:UYVY8_2X8/1920x1080 field:none colorspace:unknown]
>   <- "ipu3-imgu 1 parameters":0 []
>   pad2: Source
>   [fmt:UYVY8_2X8/1920x1080 field:none colorspace:unknown]
>   -> "ipu3-imgu 1 output":0 []
>   pad3: Source
>   

Re: [PATCH v7 00/16] Intel IPU3 ImgU patchset

2018-11-13 Thread Sakari Ailus
On Tue, Nov 13, 2018 at 07:04:01PM +0800, Bing Bu Cao wrote:
> 
> 
> On 11/13/2018 06:31 PM, Sakari Ailus wrote:
> > Hi Bing Bu,
> >
> > On Mon, Nov 12, 2018 at 12:31:16PM +0800, Bing Bu Cao wrote:
> >>
> >> On 11/09/2018 06:09 PM, Sakari Ailus wrote:
> >>> Hi Bing Bu,
> >>>
> >>> On Wed, Nov 07, 2018 at 12:16:47PM +0800, Bing Bu Cao wrote:
>  On 11/01/2018 08:03 PM, Sakari Ailus wrote:
> > Hi Yong,
> >
> > Thanks for the update!
> >
> > On Mon, Oct 29, 2018 at 03:22:54PM -0700, Yong Zhi wrote:
> >> Hi,
> >>
> >> This series adds support for the Intel IPU3 (Image Processing Unit)
> >> ImgU which is essentially a modern memory-to-memory ISP. It implements
> >> raw Bayer to YUV image format conversion as well as a large number of
> >> other pixel processing algorithms for improving the image quality.
> >>
> >> Meta data formats are defined for image statistics (3A, i.e. automatic
> >> white balance, exposure and focus, histogram and local area contrast
> >> enhancement) as well as for the pixel processing algorithm parameters.
> >> The documentation for these formats is currently not included in the
> >> patchset but will be added in a future version of this set.
> >>
> >> The algorithm parameters need to be considered specific to a given 
> >> frame
> >> and typically a large number of these parameters change on frame to 
> >> frame
> >> basis. Additionally, the parameters are highly structured (and not a 
> >> flat
> >> space of independent configuration primitives). They also reflect the
> >> data structures used by the firmware and the hardware. On top of that,
> >> the algorithms require highly specialized user space to make meaningful
> >> use of them. For these reasons it has been chosen video buffers to pass
> >> the parameters to the device.
> >>
> >> On individual patches:
> >>
> >> The heart of ImgU is the CSS, or Camera Subsystem, which contains the
> >> image processors and HW accelerators.
> >>
> >> The 3A statistics and other firmware parameter computation related
> >> functions are implemented in patch 11.
> >>
> >> All IPU3 pipeline default settings can be found in patch 10.
> >>
> >> To access DDR via ImgU's own memory space, IPU3 is also equipped with
> >> its own MMU unit, the driver is implemented in patch 6.
> >>
> >> Patch 7 uses above driver for DMA mapping operation.
> >>
> >> The communication between IPU3 firmware and driver is implemented with 
> >> circular
> >> queues in patch 8.
> >>
> >> Patch 9 provide some utility functions and manage IPU3 fw download and
> >> install.
> >>
> >> The firmware which is called ipu3-fw.bin can be downloaded from:
> >>
> >> git://git.kernel.org/pub/scm/linux/kernel/git/firmware/linux-firmware.git
> >> (commit 2c27b0cb02f18c022d8378e0e1abaf8b7ae8188f)
> >>
> >> Firmware ABI is defined in patches 4 and 5.
> >>
> >> Patches 12 and 13 are of the same file, the former contains all h/w 
> >> programming
> >> related code, the latter implements interface functions for access fw 
> >> & hw
> >> capabilities.
> >>
> >> Patch 14 has a dependency on Sakari's V4L2_BUF_TYPE_META_OUTPUT work:
> >>
> >> https://patchwork.kernel.org/patch/9976295/>
> > I've pushed the latest set here:
> >
> > https://git.linuxtv.org/sailus/media_tree.git/log/?h=meta-output>
> >
> > You can just say the entire set depends on those going forward; the
> > documentation is needed, too.
> >
> >> Patch 15 represents the top level that glues all of the other 
> >> components together,
> >> passing arguments between the components.
> >>
> >> Patch 16 is a recent effort to extend v6 for advanced camera features 
> >> like
> >> Continuous View Finder (CVF) and Snapshot During Video(SDV) support.
> >>
> >> Link to user space implementation:
> >>
> >> git clone 
> >> https://chromium.googlesource.com/chromiumos/platform/arc-camera
> >>
> >> ImgU media topology print:
> >>
> >> # media-ctl -d /dev/media0 -p
> >> Media controller API version 4.19.0
> >>
> >> Media device information
> >> 
> >> driver  ipu3-imgu
> >> model   ipu3-imgu
> >> serial  
> >> bus infoPCI::00:05.0
> >> hw revision 0x80862015
> >> driver version  4.19.0
> >>
> >> Device topology
> >> - entity 1: ipu3-imgu 0 (5 pads, 5 links)
> >> type V4L2 subdev subtype Unknown flags 0
> >> device node name /dev/v4l-subdev0
> >>pad0: Sink
> >>[fmt:UYVY8_2X8/1920x1080 field:none colorspace:unknown
> > This doesn't seem right. Which formats can be enumerated from the pad?
> >>> Looking at the 

Re: [PATCH v7 00/16] Intel IPU3 ImgU patchset

2018-11-13 Thread Bing Bu Cao



On 11/13/2018 06:31 PM, Sakari Ailus wrote:
> Hi Bing Bu,
>
> On Mon, Nov 12, 2018 at 12:31:16PM +0800, Bing Bu Cao wrote:
>>
>> On 11/09/2018 06:09 PM, Sakari Ailus wrote:
>>> Hi Bing Bu,
>>>
>>> On Wed, Nov 07, 2018 at 12:16:47PM +0800, Bing Bu Cao wrote:
 On 11/01/2018 08:03 PM, Sakari Ailus wrote:
> Hi Yong,
>
> Thanks for the update!
>
> On Mon, Oct 29, 2018 at 03:22:54PM -0700, Yong Zhi wrote:
>> Hi,
>>
>> This series adds support for the Intel IPU3 (Image Processing Unit)
>> ImgU which is essentially a modern memory-to-memory ISP. It implements
>> raw Bayer to YUV image format conversion as well as a large number of
>> other pixel processing algorithms for improving the image quality.
>>
>> Meta data formats are defined for image statistics (3A, i.e. automatic
>> white balance, exposure and focus, histogram and local area contrast
>> enhancement) as well as for the pixel processing algorithm parameters.
>> The documentation for these formats is currently not included in the
>> patchset but will be added in a future version of this set.
>>
>> The algorithm parameters need to be considered specific to a given frame
>> and typically a large number of these parameters change on frame to frame
>> basis. Additionally, the parameters are highly structured (and not a flat
>> space of independent configuration primitives). They also reflect the
>> data structures used by the firmware and the hardware. On top of that,
>> the algorithms require highly specialized user space to make meaningful
>> use of them. For these reasons it has been chosen video buffers to pass
>> the parameters to the device.
>>
>> On individual patches:
>>
>> The heart of ImgU is the CSS, or Camera Subsystem, which contains the
>> image processors and HW accelerators.
>>
>> The 3A statistics and other firmware parameter computation related
>> functions are implemented in patch 11.
>>
>> All IPU3 pipeline default settings can be found in patch 10.
>>
>> To access DDR via ImgU's own memory space, IPU3 is also equipped with
>> its own MMU unit, the driver is implemented in patch 6.
>>
>> Patch 7 uses above driver for DMA mapping operation.
>>
>> The communication between IPU3 firmware and driver is implemented with 
>> circular
>> queues in patch 8.
>>
>> Patch 9 provide some utility functions and manage IPU3 fw download and
>> install.
>>
>> The firmware which is called ipu3-fw.bin can be downloaded from:
>>
>> git://git.kernel.org/pub/scm/linux/kernel/git/firmware/linux-firmware.git
>> (commit 2c27b0cb02f18c022d8378e0e1abaf8b7ae8188f)
>>
>> Firmware ABI is defined in patches 4 and 5.
>>
>> Patches 12 and 13 are of the same file, the former contains all h/w 
>> programming
>> related code, the latter implements interface functions for access fw & 
>> hw
>> capabilities.
>>
>> Patch 14 has a dependency on Sakari's V4L2_BUF_TYPE_META_OUTPUT work:
>>
>> https://patchwork.kernel.org/patch/9976295/>
> I've pushed the latest set here:
>
> https://git.linuxtv.org/sailus/media_tree.git/log/?h=meta-output>
>
> You can just say the entire set depends on those going forward; the
> documentation is needed, too.
>
>> Patch 15 represents the top level that glues all of the other components 
>> together,
>> passing arguments between the components.
>>
>> Patch 16 is a recent effort to extend v6 for advanced camera features 
>> like
>> Continuous View Finder (CVF) and Snapshot During Video(SDV) support.
>>
>> Link to user space implementation:
>>
>> git clone 
>> https://chromium.googlesource.com/chromiumos/platform/arc-camera
>>
>> ImgU media topology print:
>>
>> # media-ctl -d /dev/media0 -p
>> Media controller API version 4.19.0
>>
>> Media device information
>> 
>> driver  ipu3-imgu
>> model   ipu3-imgu
>> serial  
>> bus infoPCI::00:05.0
>> hw revision 0x80862015
>> driver version  4.19.0
>>
>> Device topology
>> - entity 1: ipu3-imgu 0 (5 pads, 5 links)
>> type V4L2 subdev subtype Unknown flags 0
>> device node name /dev/v4l-subdev0
>>  pad0: Sink
>>  [fmt:UYVY8_2X8/1920x1080 field:none colorspace:unknown
> This doesn't seem right. Which formats can be enumerated from the pad?
>>> Looking at the code, the OUTPUT video nodes have 10-bit GRBG (or a variant)
>>> format whereas the CAPTURE video nodes always have NV12. Can you confirm?
>> Hi, Sakari,
>> Yes, I think the pad_fmt should also be changed.
>> Yong, could you add some extra code for this and test? like:
>>
>> static int ipu3_v4l2_node_setup(struct imgu_device *imgu, 

Re: [PATCH v7 00/16] Intel IPU3 ImgU patchset

2018-11-13 Thread Sakari Ailus
Hi Bing Bu,

On Mon, Nov 12, 2018 at 12:31:16PM +0800, Bing Bu Cao wrote:
> 
> 
> On 11/09/2018 06:09 PM, Sakari Ailus wrote:
> > Hi Bing Bu,
> >
> > On Wed, Nov 07, 2018 at 12:16:47PM +0800, Bing Bu Cao wrote:
> >> On 11/01/2018 08:03 PM, Sakari Ailus wrote:
> >>> Hi Yong,
> >>>
> >>> Thanks for the update!
> >>>
> >>> On Mon, Oct 29, 2018 at 03:22:54PM -0700, Yong Zhi wrote:
>  Hi,
> 
>  This series adds support for the Intel IPU3 (Image Processing Unit)
>  ImgU which is essentially a modern memory-to-memory ISP. It implements
>  raw Bayer to YUV image format conversion as well as a large number of
>  other pixel processing algorithms for improving the image quality.
> 
>  Meta data formats are defined for image statistics (3A, i.e. automatic
>  white balance, exposure and focus, histogram and local area contrast
>  enhancement) as well as for the pixel processing algorithm parameters.
>  The documentation for these formats is currently not included in the
>  patchset but will be added in a future version of this set.
> 
>  The algorithm parameters need to be considered specific to a given frame
>  and typically a large number of these parameters change on frame to frame
>  basis. Additionally, the parameters are highly structured (and not a flat
>  space of independent configuration primitives). They also reflect the
>  data structures used by the firmware and the hardware. On top of that,
>  the algorithms require highly specialized user space to make meaningful
>  use of them. For these reasons it has been chosen video buffers to pass
>  the parameters to the device.
> 
>  On individual patches:
> 
>  The heart of ImgU is the CSS, or Camera Subsystem, which contains the
>  image processors and HW accelerators.
> 
>  The 3A statistics and other firmware parameter computation related
>  functions are implemented in patch 11.
> 
>  All IPU3 pipeline default settings can be found in patch 10.
> 
>  To access DDR via ImgU's own memory space, IPU3 is also equipped with
>  its own MMU unit, the driver is implemented in patch 6.
> 
>  Patch 7 uses above driver for DMA mapping operation.
> 
>  The communication between IPU3 firmware and driver is implemented with 
>  circular
>  queues in patch 8.
> 
>  Patch 9 provide some utility functions and manage IPU3 fw download and
>  install.
> 
>  The firmware which is called ipu3-fw.bin can be downloaded from:
> 
>  git://git.kernel.org/pub/scm/linux/kernel/git/firmware/linux-firmware.git
>  (commit 2c27b0cb02f18c022d8378e0e1abaf8b7ae8188f)
> 
>  Firmware ABI is defined in patches 4 and 5.
> 
>  Patches 12 and 13 are of the same file, the former contains all h/w 
>  programming
>  related code, the latter implements interface functions for access fw & 
>  hw
>  capabilities.
> 
>  Patch 14 has a dependency on Sakari's V4L2_BUF_TYPE_META_OUTPUT work:
> 
>  https://patchwork.kernel.org/patch/9976295/>
> >>> I've pushed the latest set here:
> >>>
> >>> https://git.linuxtv.org/sailus/media_tree.git/log/?h=meta-output>
> >>>
> >>> You can just say the entire set depends on those going forward; the
> >>> documentation is needed, too.
> >>>
>  Patch 15 represents the top level that glues all of the other components 
>  together,
>  passing arguments between the components.
> 
>  Patch 16 is a recent effort to extend v6 for advanced camera features 
>  like
>  Continuous View Finder (CVF) and Snapshot During Video(SDV) support.
> 
>  Link to user space implementation:
> 
>  git clone 
>  https://chromium.googlesource.com/chromiumos/platform/arc-camera
> 
>  ImgU media topology print:
> 
>  # media-ctl -d /dev/media0 -p
>  Media controller API version 4.19.0
> 
>  Media device information
>  
>  driver  ipu3-imgu
>  model   ipu3-imgu
>  serial  
>  bus infoPCI::00:05.0
>  hw revision 0x80862015
>  driver version  4.19.0
> 
>  Device topology
>  - entity 1: ipu3-imgu 0 (5 pads, 5 links)
>  type V4L2 subdev subtype Unknown flags 0
>  device node name /dev/v4l-subdev0
>   pad0: Sink
>   [fmt:UYVY8_2X8/1920x1080 field:none colorspace:unknown
> >>> This doesn't seem right. Which formats can be enumerated from the pad?
> > Looking at the code, the OUTPUT video nodes have 10-bit GRBG (or a variant)
> > format whereas the CAPTURE video nodes always have NV12. Can you confirm?
> Hi, Sakari,
> Yes, I think the pad_fmt should also be changed.
> Yong, could you add some extra code for this and test? like:
> 
> static int ipu3_v4l2_node_setup(struct imgu_device *imgu, unsigned int pipe,
> ...
>     

Re: [PATCH v7 00/16] Intel IPU3 ImgU patchset

2018-11-11 Thread Bing Bu Cao



On 11/09/2018 06:09 PM, Sakari Ailus wrote:
> Hi Bing Bu,
>
> On Wed, Nov 07, 2018 at 12:16:47PM +0800, Bing Bu Cao wrote:
>> On 11/01/2018 08:03 PM, Sakari Ailus wrote:
>>> Hi Yong,
>>>
>>> Thanks for the update!
>>>
>>> On Mon, Oct 29, 2018 at 03:22:54PM -0700, Yong Zhi wrote:
 Hi,

 This series adds support for the Intel IPU3 (Image Processing Unit)
 ImgU which is essentially a modern memory-to-memory ISP. It implements
 raw Bayer to YUV image format conversion as well as a large number of
 other pixel processing algorithms for improving the image quality.

 Meta data formats are defined for image statistics (3A, i.e. automatic
 white balance, exposure and focus, histogram and local area contrast
 enhancement) as well as for the pixel processing algorithm parameters.
 The documentation for these formats is currently not included in the
 patchset but will be added in a future version of this set.

 The algorithm parameters need to be considered specific to a given frame
 and typically a large number of these parameters change on frame to frame
 basis. Additionally, the parameters are highly structured (and not a flat
 space of independent configuration primitives). They also reflect the
 data structures used by the firmware and the hardware. On top of that,
 the algorithms require highly specialized user space to make meaningful
 use of them. For these reasons it has been chosen video buffers to pass
 the parameters to the device.

 On individual patches:

 The heart of ImgU is the CSS, or Camera Subsystem, which contains the
 image processors and HW accelerators.

 The 3A statistics and other firmware parameter computation related
 functions are implemented in patch 11.

 All IPU3 pipeline default settings can be found in patch 10.

 To access DDR via ImgU's own memory space, IPU3 is also equipped with
 its own MMU unit, the driver is implemented in patch 6.

 Patch 7 uses above driver for DMA mapping operation.

 The communication between IPU3 firmware and driver is implemented with 
 circular
 queues in patch 8.

 Patch 9 provide some utility functions and manage IPU3 fw download and
 install.

 The firmware which is called ipu3-fw.bin can be downloaded from:

 git://git.kernel.org/pub/scm/linux/kernel/git/firmware/linux-firmware.git
 (commit 2c27b0cb02f18c022d8378e0e1abaf8b7ae8188f)

 Firmware ABI is defined in patches 4 and 5.

 Patches 12 and 13 are of the same file, the former contains all h/w 
 programming
 related code, the latter implements interface functions for access fw & hw
 capabilities.

 Patch 14 has a dependency on Sakari's V4L2_BUF_TYPE_META_OUTPUT work:

 https://patchwork.kernel.org/patch/9976295/>
>>> I've pushed the latest set here:
>>>
>>> https://git.linuxtv.org/sailus/media_tree.git/log/?h=meta-output>
>>>
>>> You can just say the entire set depends on those going forward; the
>>> documentation is needed, too.
>>>
 Patch 15 represents the top level that glues all of the other components 
 together,
 passing arguments between the components.

 Patch 16 is a recent effort to extend v6 for advanced camera features like
 Continuous View Finder (CVF) and Snapshot During Video(SDV) support.

 Link to user space implementation:

 git clone https://chromium.googlesource.com/chromiumos/platform/arc-camera

 ImgU media topology print:

 # media-ctl -d /dev/media0 -p
 Media controller API version 4.19.0

 Media device information
 
 driver  ipu3-imgu
 model   ipu3-imgu
 serial  
 bus infoPCI::00:05.0
 hw revision 0x80862015
 driver version  4.19.0

 Device topology
 - entity 1: ipu3-imgu 0 (5 pads, 5 links)
 type V4L2 subdev subtype Unknown flags 0
 device node name /dev/v4l-subdev0
pad0: Sink
[fmt:UYVY8_2X8/1920x1080 field:none colorspace:unknown
>>> This doesn't seem right. Which formats can be enumerated from the pad?
> Looking at the code, the OUTPUT video nodes have 10-bit GRBG (or a variant)
> format whereas the CAPTURE video nodes always have NV12. Can you confirm?
Hi, Sakari,
Yes, I think the pad_fmt should also be changed.
Yong, could you add some extra code for this and test? like:

static int ipu3_v4l2_node_setup(struct imgu_device *imgu, unsigned int pipe,
...
    V4L2_PIX_FMT_NV12;
    node->vdev_fmt.fmt.pix_mp = def_pix_fmt;
    }

+   if (node->vdev_fmt.type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE)
+   node->pad_fmt.code = MEDIA_BUS_FMT_SGRBG10_1X10;
+
 
>
> If the OUTPUT video node format selection has no effect on the rest of the
> pipeline (device capabilities, 

Re: [PATCH v7 00/16] Intel IPU3 ImgU patchset

2018-11-09 Thread Sakari Ailus
Hi Yong,

On Fri, Nov 09, 2018 at 01:28:27AM +, Zhi, Yong wrote:
> Hi, Sakari,
> 
> Thanks for your review and comments.
> Bingbu has replied to some of your questions, so I will continue with the 
> rest.
> 
> > -Original Message-
> > From: Bing Bu Cao [mailto:bingbu@linux.intel.com]
> > Sent: Tuesday, November 6, 2018 10:17 PM
> > To: Sakari Ailus ; Zhi, Yong
> > 
> > Cc: linux-media@vger.kernel.org; tf...@chromium.org;
> > mche...@kernel.org; hans.verk...@cisco.com;
> > laurent.pinch...@ideasonboard.com; Mani, Rajmohan
> > ; Zheng, Jian Xu ; Hu,
> > Jerry W ; Toivonen, Tuukka
> > ; Qiu, Tian Shu ; Cao,
> > Bingbu 
> > Subject: Re: [PATCH v7 00/16] Intel IPU3 ImgU patchset
> > 
> > 
> > On 11/01/2018 08:03 PM, Sakari Ailus wrote:
> > > Hi Yong,
> > >
> > > Thanks for the update!
> > >
> > > On Mon, Oct 29, 2018 at 03:22:54PM -0700, Yong Zhi wrote:
> > >> Hi,
> > >>
> > >> This series adds support for the Intel IPU3 (Image Processing Unit)
> > >> ImgU which is essentially a modern memory-to-memory ISP. It
> > >> implements raw Bayer to YUV image format conversion as well as a
> > >> large number of other pixel processing algorithms for improving the image
> > quality.
> > >>
> > >> Meta data formats are defined for image statistics (3A, i.e.
> > >> automatic white balance, exposure and focus, histogram and local area
> > >> contrast
> > >> enhancement) as well as for the pixel processing algorithm parameters.
> > >> The documentation for these formats is currently not included in the
> > >> patchset but will be added in a future version of this set.
> > >>
> > >> The algorithm parameters need to be considered specific to a given
> > >> frame and typically a large number of these parameters change on
> > >> frame to frame basis. Additionally, the parameters are highly
> > >> structured (and not a flat space of independent configuration
> > >> primitives). They also reflect the data structures used by the
> > >> firmware and the hardware. On top of that, the algorithms require
> > >> highly specialized user space to make meaningful use of them. For
> > >> these reasons it has been chosen video buffers to pass the parameters to
> > the device.
> > >>
> > >> On individual patches:
> > >>
> > >> The heart of ImgU is the CSS, or Camera Subsystem, which contains the
> > >> image processors and HW accelerators.
> > >>
> > >> The 3A statistics and other firmware parameter computation related
> > >> functions are implemented in patch 11.
> > >>
> > >> All IPU3 pipeline default settings can be found in patch 10.
> > >>
> > >> To access DDR via ImgU's own memory space, IPU3 is also equipped with
> > >> its own MMU unit, the driver is implemented in patch 6.
> > >>
> > >> Patch 7 uses above driver for DMA mapping operation.
> > >>
> > >> The communication between IPU3 firmware and driver is implemented
> > >> with circular queues in patch 8.
> > >>
> > >> Patch 9 provide some utility functions and manage IPU3 fw download
> > >> and install.
> > >>
> > >> The firmware which is called ipu3-fw.bin can be downloaded from:
> > >>
> > >> git://git.kernel.org/pub/scm/linux/kernel/git/firmware/linux-firmware
> > >> .git (commit 2c27b0cb02f18c022d8378e0e1abaf8b7ae8188f)
> > >>
> > >> Firmware ABI is defined in patches 4 and 5.
> > >>
> > >> Patches 12 and 13 are of the same file, the former contains all h/w
> > >> programming related code, the latter implements interface functions
> > >> for access fw & hw capabilities.
> > >>
> > >> Patch 14 has a dependency on Sakari's V4L2_BUF_TYPE_META_OUTPUT
> > work:
> > >>
> > >> https://patchwork.kernel.org/patch/9976295/>
> > > I've pushed the latest set here:
> > >
> > > https://git.linuxtv.org/sailus/media_tree.git/log/?h=meta-output>
> > >
> > > You can just say the entire set depends on those going forward; the
> > > documentation is needed, too.
> > >
> 
> Ack.
> 
> > >> Patch 15 represents the top level that glues all of the other
> > >> components together, passing arguments between the components.

Re: [PATCH v7 00/16] Intel IPU3 ImgU patchset

2018-11-09 Thread Sakari Ailus
Hi Bing Bu,

On Wed, Nov 07, 2018 at 12:16:47PM +0800, Bing Bu Cao wrote:
> 
> On 11/01/2018 08:03 PM, Sakari Ailus wrote:
> > Hi Yong,
> >
> > Thanks for the update!
> >
> > On Mon, Oct 29, 2018 at 03:22:54PM -0700, Yong Zhi wrote:
> >> Hi,
> >>
> >> This series adds support for the Intel IPU3 (Image Processing Unit)
> >> ImgU which is essentially a modern memory-to-memory ISP. It implements
> >> raw Bayer to YUV image format conversion as well as a large number of
> >> other pixel processing algorithms for improving the image quality.
> >>
> >> Meta data formats are defined for image statistics (3A, i.e. automatic
> >> white balance, exposure and focus, histogram and local area contrast
> >> enhancement) as well as for the pixel processing algorithm parameters.
> >> The documentation for these formats is currently not included in the
> >> patchset but will be added in a future version of this set.
> >>
> >> The algorithm parameters need to be considered specific to a given frame
> >> and typically a large number of these parameters change on frame to frame
> >> basis. Additionally, the parameters are highly structured (and not a flat
> >> space of independent configuration primitives). They also reflect the
> >> data structures used by the firmware and the hardware. On top of that,
> >> the algorithms require highly specialized user space to make meaningful
> >> use of them. For these reasons it has been chosen video buffers to pass
> >> the parameters to the device.
> >>
> >> On individual patches:
> >>
> >> The heart of ImgU is the CSS, or Camera Subsystem, which contains the
> >> image processors and HW accelerators.
> >>
> >> The 3A statistics and other firmware parameter computation related
> >> functions are implemented in patch 11.
> >>
> >> All IPU3 pipeline default settings can be found in patch 10.
> >>
> >> To access DDR via ImgU's own memory space, IPU3 is also equipped with
> >> its own MMU unit, the driver is implemented in patch 6.
> >>
> >> Patch 7 uses above driver for DMA mapping operation.
> >>
> >> The communication between IPU3 firmware and driver is implemented with 
> >> circular
> >> queues in patch 8.
> >>
> >> Patch 9 provide some utility functions and manage IPU3 fw download and
> >> install.
> >>
> >> The firmware which is called ipu3-fw.bin can be downloaded from:
> >>
> >> git://git.kernel.org/pub/scm/linux/kernel/git/firmware/linux-firmware.git
> >> (commit 2c27b0cb02f18c022d8378e0e1abaf8b7ae8188f)
> >>
> >> Firmware ABI is defined in patches 4 and 5.
> >>
> >> Patches 12 and 13 are of the same file, the former contains all h/w 
> >> programming
> >> related code, the latter implements interface functions for access fw & hw
> >> capabilities.
> >>
> >> Patch 14 has a dependency on Sakari's V4L2_BUF_TYPE_META_OUTPUT work:
> >>
> >> https://patchwork.kernel.org/patch/9976295/>
> > I've pushed the latest set here:
> >
> > https://git.linuxtv.org/sailus/media_tree.git/log/?h=meta-output>
> >
> > You can just say the entire set depends on those going forward; the
> > documentation is needed, too.
> >
> >> Patch 15 represents the top level that glues all of the other components 
> >> together,
> >> passing arguments between the components.
> >>
> >> Patch 16 is a recent effort to extend v6 for advanced camera features like
> >> Continuous View Finder (CVF) and Snapshot During Video(SDV) support.
> >>
> >> Link to user space implementation:
> >>
> >> git clone https://chromium.googlesource.com/chromiumos/platform/arc-camera
> >>
> >> ImgU media topology print:
> >>
> >> # media-ctl -d /dev/media0 -p
> >> Media controller API version 4.19.0
> >>
> >> Media device information
> >> 
> >> driver  ipu3-imgu
> >> model   ipu3-imgu
> >> serial  
> >> bus infoPCI::00:05.0
> >> hw revision 0x80862015
> >> driver version  4.19.0
> >>
> >> Device topology
> >> - entity 1: ipu3-imgu 0 (5 pads, 5 links)
> >> type V4L2 subdev subtype Unknown flags 0
> >> device node name /dev/v4l-subdev0
> >>pad0: Sink
> >>[fmt:UYVY8_2X8/1920x1080 field:none colorspace:unknown
> > This doesn't seem right. Which formats can be enumerated from the pad?

Looking at the code, the OUTPUT video nodes have 10-bit GRBG (or a variant)
format whereas the CAPTURE video nodes always have NV12. Can you confirm?

If the OUTPUT video node format selection has no effect on the rest of the
pipeline (device capabilities, which processing blocks are in use, CAPTURE
video nodes formats etc.), I think you could simply use the FIXED media bus
code for each pad. That would actually make sense: this device always works
from memory to memory, and thus does not really have a pixel data bus
external to the device which is what the media bus codes really are for.

> >
> >> crop:(0,0)/1920x1080
> >> compose:(0,0)/1920x1080]
> > Does the compose rectangle affect the scaling on all outputs?
> Sakari, driver use 

RE: [PATCH v7 00/16] Intel IPU3 ImgU patchset

2018-11-08 Thread Zhi, Yong
Hi, Sakari,

Thanks for your review and comments.
Bingbu has replied to some of your questions, so I will continue with the rest.

> -Original Message-
> From: Bing Bu Cao [mailto:bingbu@linux.intel.com]
> Sent: Tuesday, November 6, 2018 10:17 PM
> To: Sakari Ailus ; Zhi, Yong
> 
> Cc: linux-media@vger.kernel.org; tf...@chromium.org;
> mche...@kernel.org; hans.verk...@cisco.com;
> laurent.pinch...@ideasonboard.com; Mani, Rajmohan
> ; Zheng, Jian Xu ; Hu,
> Jerry W ; Toivonen, Tuukka
> ; Qiu, Tian Shu ; Cao,
> Bingbu 
> Subject: Re: [PATCH v7 00/16] Intel IPU3 ImgU patchset
> 
> 
> On 11/01/2018 08:03 PM, Sakari Ailus wrote:
> > Hi Yong,
> >
> > Thanks for the update!
> >
> > On Mon, Oct 29, 2018 at 03:22:54PM -0700, Yong Zhi wrote:
> >> Hi,
> >>
> >> This series adds support for the Intel IPU3 (Image Processing Unit)
> >> ImgU which is essentially a modern memory-to-memory ISP. It
> >> implements raw Bayer to YUV image format conversion as well as a
> >> large number of other pixel processing algorithms for improving the image
> quality.
> >>
> >> Meta data formats are defined for image statistics (3A, i.e.
> >> automatic white balance, exposure and focus, histogram and local area
> >> contrast
> >> enhancement) as well as for the pixel processing algorithm parameters.
> >> The documentation for these formats is currently not included in the
> >> patchset but will be added in a future version of this set.
> >>
> >> The algorithm parameters need to be considered specific to a given
> >> frame and typically a large number of these parameters change on
> >> frame to frame basis. Additionally, the parameters are highly
> >> structured (and not a flat space of independent configuration
> >> primitives). They also reflect the data structures used by the
> >> firmware and the hardware. On top of that, the algorithms require
> >> highly specialized user space to make meaningful use of them. For
> >> these reasons it has been chosen video buffers to pass the parameters to
> the device.
> >>
> >> On individual patches:
> >>
> >> The heart of ImgU is the CSS, or Camera Subsystem, which contains the
> >> image processors and HW accelerators.
> >>
> >> The 3A statistics and other firmware parameter computation related
> >> functions are implemented in patch 11.
> >>
> >> All IPU3 pipeline default settings can be found in patch 10.
> >>
> >> To access DDR via ImgU's own memory space, IPU3 is also equipped with
> >> its own MMU unit, the driver is implemented in patch 6.
> >>
> >> Patch 7 uses above driver for DMA mapping operation.
> >>
> >> The communication between IPU3 firmware and driver is implemented
> >> with circular queues in patch 8.
> >>
> >> Patch 9 provide some utility functions and manage IPU3 fw download
> >> and install.
> >>
> >> The firmware which is called ipu3-fw.bin can be downloaded from:
> >>
> >> git://git.kernel.org/pub/scm/linux/kernel/git/firmware/linux-firmware
> >> .git (commit 2c27b0cb02f18c022d8378e0e1abaf8b7ae8188f)
> >>
> >> Firmware ABI is defined in patches 4 and 5.
> >>
> >> Patches 12 and 13 are of the same file, the former contains all h/w
> >> programming related code, the latter implements interface functions
> >> for access fw & hw capabilities.
> >>
> >> Patch 14 has a dependency on Sakari's V4L2_BUF_TYPE_META_OUTPUT
> work:
> >>
> >> https://patchwork.kernel.org/patch/9976295/>
> > I've pushed the latest set here:
> >
> > https://git.linuxtv.org/sailus/media_tree.git/log/?h=meta-output>
> >
> > You can just say the entire set depends on those going forward; the
> > documentation is needed, too.
> >

Ack.

> >> Patch 15 represents the top level that glues all of the other
> >> components together, passing arguments between the components.
> >>
> >> Patch 16 is a recent effort to extend v6 for advanced camera features
> >> like Continuous View Finder (CVF) and Snapshot During Video(SDV)
> support.
> >>
> >> Link to user space implementation:
> >>
> >> git clone
> >> https://chromium.googlesource.com/chromiumos/platform/arc-camera
> >>
> >> ImgU media topology print:
> >>
> >> # media-ctl -d /dev/media0 -p
> >> Media controller API version 4.19.0
> >>
> >> Media d

Re: [PATCH v7 00/16] Intel IPU3 ImgU patchset

2018-11-06 Thread Bing Bu Cao


On 11/01/2018 08:03 PM, Sakari Ailus wrote:
> Hi Yong,
>
> Thanks for the update!
>
> On Mon, Oct 29, 2018 at 03:22:54PM -0700, Yong Zhi wrote:
>> Hi,
>>
>> This series adds support for the Intel IPU3 (Image Processing Unit)
>> ImgU which is essentially a modern memory-to-memory ISP. It implements
>> raw Bayer to YUV image format conversion as well as a large number of
>> other pixel processing algorithms for improving the image quality.
>>
>> Meta data formats are defined for image statistics (3A, i.e. automatic
>> white balance, exposure and focus, histogram and local area contrast
>> enhancement) as well as for the pixel processing algorithm parameters.
>> The documentation for these formats is currently not included in the
>> patchset but will be added in a future version of this set.
>>
>> The algorithm parameters need to be considered specific to a given frame
>> and typically a large number of these parameters change on frame to frame
>> basis. Additionally, the parameters are highly structured (and not a flat
>> space of independent configuration primitives). They also reflect the
>> data structures used by the firmware and the hardware. On top of that,
>> the algorithms require highly specialized user space to make meaningful
>> use of them. For these reasons it has been chosen video buffers to pass
>> the parameters to the device.
>>
>> On individual patches:
>>
>> The heart of ImgU is the CSS, or Camera Subsystem, which contains the
>> image processors and HW accelerators.
>>
>> The 3A statistics and other firmware parameter computation related
>> functions are implemented in patch 11.
>>
>> All IPU3 pipeline default settings can be found in patch 10.
>>
>> To access DDR via ImgU's own memory space, IPU3 is also equipped with
>> its own MMU unit, the driver is implemented in patch 6.
>>
>> Patch 7 uses above driver for DMA mapping operation.
>>
>> The communication between IPU3 firmware and driver is implemented with 
>> circular
>> queues in patch 8.
>>
>> Patch 9 provide some utility functions and manage IPU3 fw download and
>> install.
>>
>> The firmware which is called ipu3-fw.bin can be downloaded from:
>>
>> git://git.kernel.org/pub/scm/linux/kernel/git/firmware/linux-firmware.git
>> (commit 2c27b0cb02f18c022d8378e0e1abaf8b7ae8188f)
>>
>> Firmware ABI is defined in patches 4 and 5.
>>
>> Patches 12 and 13 are of the same file, the former contains all h/w 
>> programming
>> related code, the latter implements interface functions for access fw & hw
>> capabilities.
>>
>> Patch 14 has a dependency on Sakari's V4L2_BUF_TYPE_META_OUTPUT work:
>>
>> https://patchwork.kernel.org/patch/9976295/>
> I've pushed the latest set here:
>
> https://git.linuxtv.org/sailus/media_tree.git/log/?h=meta-output>
>
> You can just say the entire set depends on those going forward; the
> documentation is needed, too.
>
>> Patch 15 represents the top level that glues all of the other components 
>> together,
>> passing arguments between the components.
>>
>> Patch 16 is a recent effort to extend v6 for advanced camera features like
>> Continuous View Finder (CVF) and Snapshot During Video(SDV) support.
>>
>> Link to user space implementation:
>>
>> git clone https://chromium.googlesource.com/chromiumos/platform/arc-camera
>>
>> ImgU media topology print:
>>
>> # media-ctl -d /dev/media0 -p
>> Media controller API version 4.19.0
>>
>> Media device information
>> 
>> driver  ipu3-imgu
>> model   ipu3-imgu
>> serial  
>> bus infoPCI::00:05.0
>> hw revision 0x80862015
>> driver version  4.19.0
>>
>> Device topology
>> - entity 1: ipu3-imgu 0 (5 pads, 5 links)
>> type V4L2 subdev subtype Unknown flags 0
>> device node name /dev/v4l-subdev0
>>  pad0: Sink
>>  [fmt:UYVY8_2X8/1920x1080 field:none colorspace:unknown
> This doesn't seem right. Which formats can be enumerated from the pad?
>
>>   crop:(0,0)/1920x1080
>>   compose:(0,0)/1920x1080]
> Does the compose rectangle affect the scaling on all outputs?
Sakari, driver use crop and compose targets to help set input-feeder and BDS
output resolutions which are 2 key block of whole imaging pipeline, not the
actual ending output, but they will impact the final output.
>
>>  <- "ipu3-imgu 0 input":0 []
> Are there links that have no useful link configuration? If so, you should
> set them enabled and immutable in the driver.
The enabled status of input pads is used to get which pipe that user is
trying to enable (ipu3_link_setup()), so it could not been set as immutable.
>
>>  pad1: Sink
>>  [fmt:UYVY8_2X8/1920x1080 field:none colorspace:unknown]
> I'd suggest to use MEDIA_BUS_FMT_FIXED here.
>
>>  <- "ipu3-imgu 0 parameters":0 []
>>  pad2: Source
>>  [fmt:UYVY8_2X8/1920x1080 field:none colorspace:unknown]
>>  -> "ipu3-imgu 0 output":0 []
>>  pad3: Source
>>  

Re: [PATCH v7 00/16] Intel IPU3 ImgU patchset

2018-11-01 Thread Sakari Ailus
Hi Yong,

Thanks for the update!

On Mon, Oct 29, 2018 at 03:22:54PM -0700, Yong Zhi wrote:
> Hi,
> 
> This series adds support for the Intel IPU3 (Image Processing Unit)
> ImgU which is essentially a modern memory-to-memory ISP. It implements
> raw Bayer to YUV image format conversion as well as a large number of
> other pixel processing algorithms for improving the image quality.
> 
> Meta data formats are defined for image statistics (3A, i.e. automatic
> white balance, exposure and focus, histogram and local area contrast
> enhancement) as well as for the pixel processing algorithm parameters.
> The documentation for these formats is currently not included in the
> patchset but will be added in a future version of this set.
> 
> The algorithm parameters need to be considered specific to a given frame
> and typically a large number of these parameters change on frame to frame
> basis. Additionally, the parameters are highly structured (and not a flat
> space of independent configuration primitives). They also reflect the
> data structures used by the firmware and the hardware. On top of that,
> the algorithms require highly specialized user space to make meaningful
> use of them. For these reasons it has been chosen video buffers to pass
> the parameters to the device.
> 
> On individual patches:
> 
> The heart of ImgU is the CSS, or Camera Subsystem, which contains the
> image processors and HW accelerators.
> 
> The 3A statistics and other firmware parameter computation related
> functions are implemented in patch 11.
> 
> All IPU3 pipeline default settings can be found in patch 10.
> 
> To access DDR via ImgU's own memory space, IPU3 is also equipped with
> its own MMU unit, the driver is implemented in patch 6.
> 
> Patch 7 uses above driver for DMA mapping operation.
> 
> The communication between IPU3 firmware and driver is implemented with 
> circular
> queues in patch 8.
> 
> Patch 9 provide some utility functions and manage IPU3 fw download and
> install.
> 
> The firmware which is called ipu3-fw.bin can be downloaded from:
> 
> git://git.kernel.org/pub/scm/linux/kernel/git/firmware/linux-firmware.git
> (commit 2c27b0cb02f18c022d8378e0e1abaf8b7ae8188f)
> 
> Firmware ABI is defined in patches 4 and 5.
> 
> Patches 12 and 13 are of the same file, the former contains all h/w 
> programming
> related code, the latter implements interface functions for access fw & hw
> capabilities.
> 
> Patch 14 has a dependency on Sakari's V4L2_BUF_TYPE_META_OUTPUT work:
> 
> https://patchwork.kernel.org/patch/9976295/>

I've pushed the latest set here:

https://git.linuxtv.org/sailus/media_tree.git/log/?h=meta-output>

You can just say the entire set depends on those going forward; the
documentation is needed, too.

> 
> Patch 15 represents the top level that glues all of the other components 
> together,
> passing arguments between the components.
> 
> Patch 16 is a recent effort to extend v6 for advanced camera features like
> Continuous View Finder (CVF) and Snapshot During Video(SDV) support.
> 
> Link to user space implementation:
> 
> git clone https://chromium.googlesource.com/chromiumos/platform/arc-camera
> 
> ImgU media topology print:
> 
> # media-ctl -d /dev/media0 -p
> Media controller API version 4.19.0
> 
> Media device information
> 
> driver  ipu3-imgu
> model   ipu3-imgu
> serial  
> bus infoPCI::00:05.0
> hw revision 0x80862015
> driver version  4.19.0
> 
> Device topology
> - entity 1: ipu3-imgu 0 (5 pads, 5 links)
> type V4L2 subdev subtype Unknown flags 0
> device node name /dev/v4l-subdev0
>   pad0: Sink
>   [fmt:UYVY8_2X8/1920x1080 field:none colorspace:unknown

This doesn't seem right. Which formats can be enumerated from the pad?

>crop:(0,0)/1920x1080
>compose:(0,0)/1920x1080]

Does the compose rectangle affect the scaling on all outputs?

>   <- "ipu3-imgu 0 input":0 []

Are there links that have no useful link configuration? If so, you should
set them enabled and immutable in the driver.

>   pad1: Sink
>   [fmt:UYVY8_2X8/1920x1080 field:none colorspace:unknown]

I'd suggest to use MEDIA_BUS_FMT_FIXED here.

>   <- "ipu3-imgu 0 parameters":0 []
>   pad2: Source
>   [fmt:UYVY8_2X8/1920x1080 field:none colorspace:unknown]
>   -> "ipu3-imgu 0 output":0 []
>   pad3: Source
>   [fmt:UYVY8_2X8/1920x1080 field:none colorspace:unknown]
>   -> "ipu3-imgu 0 viewfinder":0 []

Are there other differences between output and viewfinder?

>   pad4: Source
>   [fmt:UYVY8_2X8/1920x1080 field:none colorspace:unknown]
>   -> "ipu3-imgu 0 3a stat":0 []

FIXED here, too.

> 
> - entity 7: ipu3-imgu 1 (5 pads, 5 links)
> type V4L2 subdev subtype Unknown flags 0
> device node name /dev/v4l-subdev1
>   pad0: Sink
>   

[PATCH v7 00/16] Intel IPU3 ImgU patchset

2018-10-29 Thread Yong Zhi
Hi,

This series adds support for the Intel IPU3 (Image Processing Unit)
ImgU which is essentially a modern memory-to-memory ISP. It implements
raw Bayer to YUV image format conversion as well as a large number of
other pixel processing algorithms for improving the image quality.

Meta data formats are defined for image statistics (3A, i.e. automatic
white balance, exposure and focus, histogram and local area contrast
enhancement) as well as for the pixel processing algorithm parameters.
The documentation for these formats is currently not included in the
patchset but will be added in a future version of this set.

The algorithm parameters need to be considered specific to a given frame
and typically a large number of these parameters change on frame to frame
basis. Additionally, the parameters are highly structured (and not a flat
space of independent configuration primitives). They also reflect the
data structures used by the firmware and the hardware. On top of that,
the algorithms require highly specialized user space to make meaningful
use of them. For these reasons it has been chosen video buffers to pass
the parameters to the device.

On individual patches:

The heart of ImgU is the CSS, or Camera Subsystem, which contains the
image processors and HW accelerators.

The 3A statistics and other firmware parameter computation related
functions are implemented in patch 11.

All IPU3 pipeline default settings can be found in patch 10.

To access DDR via ImgU's own memory space, IPU3 is also equipped with
its own MMU unit, the driver is implemented in patch 6.

Patch 7 uses above driver for DMA mapping operation.

The communication between IPU3 firmware and driver is implemented with circular
queues in patch 8.

Patch 9 provide some utility functions and manage IPU3 fw download and
install.

The firmware which is called ipu3-fw.bin can be downloaded from:

git://git.kernel.org/pub/scm/linux/kernel/git/firmware/linux-firmware.git
(commit 2c27b0cb02f18c022d8378e0e1abaf8b7ae8188f)

Firmware ABI is defined in patches 4 and 5.

Patches 12 and 13 are of the same file, the former contains all h/w programming
related code, the latter implements interface functions for access fw & hw
capabilities.

Patch 14 has a dependency on Sakari's V4L2_BUF_TYPE_META_OUTPUT work:

https://patchwork.kernel.org/patch/9976295/>

Patch 15 represents the top level that glues all of the other components 
together,
passing arguments between the components.

Patch 16 is a recent effort to extend v6 for advanced camera features like
Continuous View Finder (CVF) and Snapshot During Video(SDV) support.

Link to user space implementation:

git clone https://chromium.googlesource.com/chromiumos/platform/arc-camera

ImgU media topology print:

# media-ctl -d /dev/media0 -p
Media controller API version 4.19.0

Media device information

driver  ipu3-imgu
model   ipu3-imgu
serial  
bus infoPCI::00:05.0
hw revision 0x80862015
driver version  4.19.0

Device topology
- entity 1: ipu3-imgu 0 (5 pads, 5 links)
type V4L2 subdev subtype Unknown flags 0
device node name /dev/v4l-subdev0
pad0: Sink
[fmt:UYVY8_2X8/1920x1080 field:none colorspace:unknown
 crop:(0,0)/1920x1080
 compose:(0,0)/1920x1080]
<- "ipu3-imgu 0 input":0 []
pad1: Sink
[fmt:UYVY8_2X8/1920x1080 field:none colorspace:unknown]
<- "ipu3-imgu 0 parameters":0 []
pad2: Source
[fmt:UYVY8_2X8/1920x1080 field:none colorspace:unknown]
-> "ipu3-imgu 0 output":0 []
pad3: Source
[fmt:UYVY8_2X8/1920x1080 field:none colorspace:unknown]
-> "ipu3-imgu 0 viewfinder":0 []
pad4: Source
[fmt:UYVY8_2X8/1920x1080 field:none colorspace:unknown]
-> "ipu3-imgu 0 3a stat":0 []

- entity 7: ipu3-imgu 1 (5 pads, 5 links)
type V4L2 subdev subtype Unknown flags 0
device node name /dev/v4l-subdev1
pad0: Sink
[fmt:UYVY8_2X8/1920x1080 field:none colorspace:unknown
 crop:(0,0)/1920x1080
 compose:(0,0)/1920x1080]
<- "ipu3-imgu 1 input":0 []
pad1: Sink
[fmt:UYVY8_2X8/1920x1080 field:none colorspace:unknown]
<- "ipu3-imgu 1 parameters":0 []
pad2: Source
[fmt:UYVY8_2X8/1920x1080 field:none colorspace:unknown]
-> "ipu3-imgu 1 output":0 []
pad3: Source
[fmt:UYVY8_2X8/1920x1080 field:none colorspace:unknown]
-> "ipu3-imgu 1 viewfinder":0 []
pad4: Source
[fmt:UYVY8_2X8/1920x1080 field:none colorspace:unknown]
-> "ipu3-imgu 1 3a stat":0 []

- entity 17: ipu3-imgu 0 input (1 pad, 1 link)
 type Node subtype V4L flags 0
 device node name