Re: [PATCH v3 16/24] media: Add i.MX media core driver

2017-02-06 Thread Steve Longerbeam 



On 02/02/2017 02:50 PM, Russell King - ARM Linux wrote:

On Fri, Jan 06, 2017 at 06:11:34PM -0800, Steve Longerbeam wrote:

+/* register an internal subdev as a platform device */
+static struct imx_media_subdev *
+add_internal_subdev(struct imx_media_dev *imxmd,
+   const struct internal_subdev *isd,
+   int ipu_id)
+{
+   struct imx_media_internal_sd_platformdata pdata;
+   struct platform_device_info pdevinfo = {0};
+   struct imx_media_subdev *imxsd;
+   struct platform_device *pdev;
+
+   switch (isd->id->grp_id) {
+   case IMX_MEDIA_GRP_ID_CAMIF0...IMX_MEDIA_GRP_ID_CAMIF1:
+   pdata.grp_id = isd->id->grp_id +
+   ((2 * ipu_id) << IMX_MEDIA_GRP_ID_CAMIF_BIT);
+   break;
+   default:
+   pdata.grp_id = isd->id->grp_id;
+   break;
+   }
+
+   /* the id of IPU this subdev will control */
+   pdata.ipu_id = ipu_id;
+
+   /* create subdev name */
+   imx_media_grp_id_to_sd_name(pdata.sd_name, sizeof(pdata.sd_name),
+   pdata.grp_id, ipu_id);
+
+   pdevinfo.name = isd->id->name;
+   pdevinfo.id = ipu_id * num_isd + isd->id->index;
+   pdevinfo.parent = imxmd->dev;
+   pdevinfo.data = 
+   pdevinfo.size_data = sizeof(pdata);
+   pdevinfo.dma_mask = DMA_BIT_MASK(32);
+
+   pdev = platform_device_register_full();
+   if (IS_ERR(pdev))
+   return ERR_CAST(pdev);
+
+   imxsd = imx_media_add_async_subdev(imxmd, NULL, dev_name(>dev));
+   if (IS_ERR(imxsd))
+   return imxsd;
+
+   imxsd->num_sink_pads = isd->num_sink_pads;
+   imxsd->num_src_pads = isd->num_src_pads;
+
+   return imxsd;
+}

You seem to create platform devices here, but I see nowhere that you
ever remove them - so if you get to the lucky point of being able to
rmmod imx-media and then try to re-insert it, you end up with a load
of kernel warnings, one for each device created this way, and
platform_device_register_full() fails:


Right, I never free the platform devices for the internal subdevs.
Fixed.

Steve

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 16/24] media: Add i.MX media core driver

2017-02-02 Thread Russell King - ARM Linux 
On Fri, Jan 06, 2017 at 06:11:34PM -0800, Steve Longerbeam wrote:
> +/* register an internal subdev as a platform device */
> +static struct imx_media_subdev *
> +add_internal_subdev(struct imx_media_dev *imxmd,
> + const struct internal_subdev *isd,
> + int ipu_id)
> +{
> + struct imx_media_internal_sd_platformdata pdata;
> + struct platform_device_info pdevinfo = {0};
> + struct imx_media_subdev *imxsd;
> + struct platform_device *pdev;
> +
> + switch (isd->id->grp_id) {
> + case IMX_MEDIA_GRP_ID_CAMIF0...IMX_MEDIA_GRP_ID_CAMIF1:
> + pdata.grp_id = isd->id->grp_id +
> + ((2 * ipu_id) << IMX_MEDIA_GRP_ID_CAMIF_BIT);
> + break;
> + default:
> + pdata.grp_id = isd->id->grp_id;
> + break;
> + }
> +
> + /* the id of IPU this subdev will control */
> + pdata.ipu_id = ipu_id;
> +
> + /* create subdev name */
> + imx_media_grp_id_to_sd_name(pdata.sd_name, sizeof(pdata.sd_name),
> + pdata.grp_id, ipu_id);
> +
> + pdevinfo.name = isd->id->name;
> + pdevinfo.id = ipu_id * num_isd + isd->id->index;
> + pdevinfo.parent = imxmd->dev;
> + pdevinfo.data = 
> + pdevinfo.size_data = sizeof(pdata);
> + pdevinfo.dma_mask = DMA_BIT_MASK(32);
> +
> + pdev = platform_device_register_full();
> + if (IS_ERR(pdev))
> + return ERR_CAST(pdev);
> +
> + imxsd = imx_media_add_async_subdev(imxmd, NULL, dev_name(>dev));
> + if (IS_ERR(imxsd))
> + return imxsd;
> +
> + imxsd->num_sink_pads = isd->num_sink_pads;
> + imxsd->num_src_pads = isd->num_src_pads;
> +
> + return imxsd;
> +}

You seem to create platform devices here, but I see nowhere that you
ever remove them - so if you get to the lucky point of being able to
rmmod imx-media and then try to re-insert it, you end up with a load
of kernel warnings, one for each device created this way, and
platform_device_register_full() fails:

WARNING: CPU: 0 PID: 2143 at /home/rmk/git/linux-rmk/fs/sysfs/dir.c:31 
sysfs_warn_dup+0x64/0x80
sysfs: cannot create duplicate filename 
'/devices/soc0/soc/soc:media@0/imx-ipuv3-smfc.2'
Modules linked in: imx_media(C+) rfcomm bnep bluetooth nfsd imx_camif(C) 
imx_ic(C) imx_smfc(C) caam_jr snd_soc_imx_sgtl5000 snd_soc_fsl_asoc_card 
uvcvideo snd_soc_imx_spdif imx_mipi_csi2(C) imx_media_common(C) 
snd_soc_imx_audmux imx219 snd_soc_sgtl5000 video_multiplexer imx_sdma caam 
imx2_wdt rc_cec coda v4l2_mem2mem videobuf2_v4l2 snd_soc_fsl_ssi 
snd_soc_fsl_spdif videobuf2_dma_contig imx_pcm_dma videobuf2_core 
videobuf2_vmalloc videobuf2_memops imx_thermal dw_hdmi_cec dw_hdmi_ahb_audio 
etnaviv fuse rc_pinnacle_pctv_hd [last unloaded: imx_media]
CPU: 0 PID: 2143 Comm: modprobe Tainted: G C  4.10.0-rc6+ #2103
Hardware name: Freescale i.MX6 Quad/DualLite (Device Tree)
Backtrace:
[] (dump_backtrace) from [] (show_stack+0x18/0x1c)
 r6:6013 r5: r4: r3:
[] (show_stack) from [] (dump_stack+0xa4/0xdc)
[] (dump_stack) from [] (__warn+0xdc/0x108)
 r6:c08ad998 r5: r4:cf4e9a78 r3:ec984980
[] (__warn) from [] (warn_slowpath_fmt+0x40/0x48)
 r10:e5800010 r8:ef1fa818 r7:ef1fa810 r6:ef202300 r5:e9809000 r4:ee868000
[] (warn_slowpath_fmt) from [] (sysfs_warn_dup+0x64/0x80)
 r3:ee868000 r2:c08ad9c0
[] (sysfs_warn_dup) from [] (sysfs_create_dir_ns+0x90/0x98)
 r6:ffef r5:ef202300 r4:eb5e3418
[] (sysfs_create_dir_ns) from [] 
(kobject_add_internal+0xa4/0x2d8)
 r6:ef1fa818 r5: r4:eb5e3418
[] (kobject_add_internal) from [] (kobject_add+0x50/0x98)
 r8:bf1f9018 r7:ef1fa810 r6:ef1fa818 r5: r4:eb5e3418
[] (kobject_add) from [] (device_add+0xc8/0x538)
 r3:ef1fa834 r2:
 r6:eb5e3418 r5: r4:eb5e3410
[] (device_add) from [] (platform_device_add+0xb0/0x214)
 r10:e5800010 r9: r8:bf1f9018 r7:e5806fd4 r6:eb5e3410 r5:eb5e3400
 r4:
[] (platform_device_add) from [] 
(platform_device_register_full+0xe8/0x110)
 r7:e5806fd4 r6:0002 r5:eb5e3400 r4:cf4e9c18
[] (platform_device_register_full) from [] 
(add_ipu_internal_subdevs+0x128/0x2c8 [imx_media])
 r5:bf1f9000 r4:0918
[] (add_ipu_internal_subdevs [imx_media]) from [] 
(imx_media_add_internal_subdevs+0x2c/0x70 [imx_media])
 r10:0048 r9:f31ceef8 r8:ef7f1d94 r7:e5800230 r6:e5800200 r5:e5800010
 r4:cf4e9c90
[] (imx_media_add_internal_subdevs [imx_media]) from [] 
(imx_media_probe+0xc4/0x1c0 [imx_media])
 r5: r4:e5800010
[] (imx_media_probe [imx_media]) from [] 
(platform_drv_probe+0x58/0xb8)
 r8: r7:bf1fbd48 r6:fdfb r5:ef1fa810 r4:fffe
[] (platform_drv_probe) from [] 
(driver_probe_device+0x204/0x2c8)
 r7:bf1fbd48 r6: r5:c1419de8 r4:ef1fa810
[] (driver_probe_device) from [] (__driver_attach+0xbc/0xc0)
 r10:0124 r8:0001 r7: r6:ef1fa844 r5:bf1fbd48 r4:ef1fa810
[] (__driver_attach) from [] (bus_for_each_dev+0x5c/0x90)
 r6:c0418d54 r5:bf1fbd48 r4:

Re: [PATCH v3 16/24] media: Add i.MX media core driver

2017-02-02 Thread Russell King - ARM Linux 
On Fri, Jan 06, 2017 at 06:11:34PM -0800, Steve Longerbeam wrote:
> +struct imx_media_dev {
> + struct media_device md;
> + struct v4l2_device  v4l2_dev;

This is similarly buggy.

struct v4l2_device {
struct device *dev;
#if defined(CONFIG_MEDIA_CONTROLLER)
struct media_device *mdev;
#endif
struct list_head subdevs;
spinlock_t lock;
char name[V4L2_DEVICE_NAME_SIZE];
void (*notify)(struct v4l2_subdev *sd,
unsigned int notification, void *arg);
struct v4l2_ctrl_handler *ctrl_handler;
struct v4l2_prio_state prio;
struct kref ref;
void (*release)(struct v4l2_device *v4l2_dev);
};

Notice the kref and release function.  This is the only way the
memory backing "struct v4l2_device" may be released.  If you wish to
embed this structure into another structure, then the lifetime of
that other structure is determined by this one.  IOW, when this
release function is called, only then may you kfree() the memory
backing struct imx_media_dev.

> + struct device *dev;

And... do you need all these struct device pointers?

imxmd->dev = dev;
imxmd->md.dev = dev;

As media_device already contains a pointer, can't you re-use that?

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 16/24] media: Add i.MX media core driver

2017-01-30 Thread Russell King - ARM Linux 
On Fri, Jan 06, 2017 at 06:11:34PM -0800, Steve Longerbeam wrote:
> Add the core media driver for i.MX SOC.
> 
> Signed-off-by: Steve Longerbeam 

Applying: media: Add i.MX media core driver
.git/rebase-apply/patch:516: new blank line at EOF.
+
.git/rebase-apply/patch:528: new blank line at EOF.
+
.git/rebase-apply/patch:556: new blank line at EOF.
+
warning: 3 lines add whitespace errors.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 16/24] media: Add i.MX media core driver

2017-01-30 Thread Russell King - ARM Linux 
On Mon, Jan 23, 2017 at 12:13:26PM +0100, Philipp Zabel wrote:
> Hi Steve,
> 
> On Sun, 2017-01-22 at 18:31 -0800, Steve Longerbeam wrote:
> > Second, ignoring the above locking issue for a moment, 
> > v4l2_pipeline_pm_use()
> > will call s_power on the sensor _first_, then the mipi csi-2 s_power, 
> > when executing
> > media-ctl -l '"ov5640 1-003c":0 -> "imx6-mipi-csi2":0[1]'. Which is the 
> > wrong order.
> > In my version which enforces the correct power on order, the mipi csi-2 
> > s_power
> > is called first in that link setup, followed by the sensor.
> 
> I don't understand why you want to power up subdevs as soon as the links
> are established. Shouldn't that rather be done for all subdevices in the
> pipeline when the corresponding capture device is opened?
> It seems to me that powering up the pipeline should be the last step
> before userspace actually starts the capture.

I agree with Philipp here - configuration of the software pipeline
shouldn't result in hardware being forced to be powered up.  That's
more of a decision for the individual sub-driver than for core.

Executing media-ctl to enable a link between two sub-device endpoints
should really be a matter of setting the software state, and when the
video device is opened for streaming, surely that's when the hardware
in the chain between the source and the capture device should be
powered up and programmed.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 16/24] media: Add i.MX media core driver

2017-01-24 Thread Philipp Zabel 
On Wed, 2017-01-18 at 17:44 -0800, Steve Longerbeam wrote:
> 
> On 01/14/2017 02:42 PM, Steve Longerbeam wrote:
> >
> >>> +/* parse inputs property from a sensor node */
> >>> +static void of_parse_sensor_inputs(struct imx_media_dev *imxmd,
> >>> +struct imx_media_subdev *sensor,
> >>> +struct device_node *sensor_np)
> >>> +{
> >>> + struct imx_media_sensor_input *sinput = >input;
> >>> + int ret, i;
> >>> +
> >>> + for (i = 0; i < IMX_MEDIA_MAX_SENSOR_INPUTS; i++) {
> >>> + const char *input_name;
> >>> + u32 val;
> >>> +
> >>> + ret = of_property_read_u32_index(sensor_np, "inputs", i, );
> >>> + if (ret)
> >>> + break;
> >>> +
> >>> + sinput->value[i] = val;
> >>> +
> >>> + ret = of_property_read_string_index(sensor_np, "input-names",
> >>> + i, _name);
> >>> + /*
> >>> +  * if input-names not provided, they will be set using
> >>> +  * the subdev name once the sensor is known during
> >>> +  * async bind
> >>> +  */
> >>> + if (!ret)
> >>> + strncpy(sinput->name[i], input_name,
> >>> + sizeof(sinput->name[i]));
> >>> + }
> >>> +
> >>> + sinput->num = i;
> >>> +
> >>> + /* if no inputs provided just assume a single input */
> >>> + if (sinput->num == 0)
> >>> + sinput->num = 1;
> >>> +}
> >> This should be parsed by the sensor driver, not imx-media.
> >
> > you're probably right. I'll submit a patch for adv7180.c.
> 
> Actually, the problem here is that this parses an input routing value to
> pass to s_routing, and an input name string. There would need to be
> another subdev callback, maybe enum_imput, that would return this
> information for the bridge driver, if this info were to be parsed and
> maintained by the sensor.
> 
> But this info should really be known and parsed by the bridge anyway,
> because as the header for s_routing states,
> 
> "An i2c device shouldn't know about whether an input pin is connected
>   to a Composite connector, because on another board or platform it
>   might be connected to something else entirely. The calling driver is
>   responsible for mapping a user-level input to the right pins on the i2c
>   device."

I think this description is for non-DT only, as parsing the DT bindings
is the obligation of the bound driver, and with that it information it
can very well know its connections.

regards
Philipp

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 16/24] media: Add i.MX media core driver

2017-01-24 Thread Philipp Zabel 
Hi Steve,

On Mon, 2017-01-23 at 17:45 -0800, Steve Longerbeam wrote:
> 
> On 01/23/2017 05:38 PM, Steve Longerbeam wrote:
> >
> >>
> >>> Second, ignoring the above locking issue for a moment,
> >>> v4l2_pipeline_pm_use()
> >>> will call s_power on the sensor _first_, then the mipi csi-2 s_power,
> >>> when executing
> >>> media-ctl -l '"ov5640 1-003c":0 -> "imx6-mipi-csi2":0[1]'. Which is the
> >>> wrong order.
> >>> In my version which enforces the correct power on order, the mipi csi-2
> >>> s_power
> >>> is called first in that link setup, followed by the sensor.
> >> I don't understand why you want to power up subdevs as soon as the links
> >> are established.
> >
> > Because that is the precedence, all other media drivers do pipeline
> > power on/off at link_notify. And v4l2_pipeline_link_notify() was written
> > as a link_notify method.
> >
> >>   Shouldn't that rather be done for all subdevices in the
> >> pipeline when the corresponding capture device is opened?
> >
> > that won't work. There's no guarantee the links will be established
> > at capture device open time.

If the device is opened before the links are established, it won't be
usable anyway. And I think the connected pipeline should be locked in
place while the video device is opened. Is there any reason to ever open
the video device and only then start linking entities?

> ugh, maybe v4l2_pipeline_pm_use() would work at open/release. If there are
> no links yet, it would basically be a no-op. And stream on requires 
> opening the
> device, and the pipeline links should be established by then, so this 
> might be
> fine, looking into this too.

Thanks for looking into it, at least I had that working for the
TC358743->MIPI-CSI2 link in my driver.

> >> It seems to me that powering up the pipeline should be the last step
> >> before userspace actually starts the capture.
> >
> > Well, I'm ok with moving pipeline power on/off to start/stop streaming.
> > I would actually prefer to do it then, I only chose at link_notify 
> > because of precedence. I'll look into it.

That might be too late, though. I would expect STREAMON/STREAMOFF to be
a rather fast operation as all the slow preparation could be at open /
REQBUFS time. Also, there might be sensors that need to be powered on to
handle the v4l2_ctrl passthrough?

regards
Philipp

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 16/24] media: Add i.MX media core driver

2017-01-23 Thread Steve Longerbeam 



On 01/23/2017 05:38 PM, Steve Longerbeam wrote:





Second, ignoring the above locking issue for a moment,
v4l2_pipeline_pm_use()
will call s_power on the sensor _first_, then the mipi csi-2 s_power,
when executing
media-ctl -l '"ov5640 1-003c":0 -> "imx6-mipi-csi2":0[1]'. Which is the
wrong order.
In my version which enforces the correct power on order, the mipi csi-2
s_power
is called first in that link setup, followed by the sensor.

I don't understand why you want to power up subdevs as soon as the links
are established.


Because that is the precedence, all other media drivers do pipeline
power on/off at link_notify. And v4l2_pipeline_link_notify() was written
as a link_notify method.


  Shouldn't that rather be done for all subdevices in the
pipeline when the corresponding capture device is opened?


that won't work. There's no guarantee the links will be established
at capture device open time.


ugh, maybe v4l2_pipeline_pm_use() would work at open/release. If there are
no links yet, it would basically be a no-op. And stream on requires 
opening the
device, and the pipeline links should be established by then, so this 
might be

fine, looking into this too.

Steve




It seems to me that powering up the pipeline should be the last step
before userspace actually starts the capture.


Well, I'm ok with moving pipeline power on/off to start/stop streaming.
I would actually prefer to do it then, I only chose at link_notify 
because

of precedence. I'll look into it.


--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 16/24] media: Add i.MX media core driver

2017-01-23 Thread Steve Longerbeam 



On 01/23/2017 03:13 AM, Philipp Zabel wrote:

Hi Steve,

On Sun, 2017-01-22 at 18:31 -0800, Steve Longerbeam wrote:

On 01/16/2017 05:47 AM, Philipp Zabel wrote:

On Sat, 2017-01-14 at 14:46 -0800, Steve Longerbeam wrote:
[...]

+Unprocessed Video Capture:
+--
+
+Send frames directly from sensor to camera interface, with no
+conversions:
+
+-> ipu_smfc -> camif

I'd call this capture interface, this is not just for cameras. Or maybe
idmac if you want to mirror hardware names?

Camif is so named because it is the V4L2 user interface for video
capture. I suppose it could be named "capif", but that doesn't role
off the tongue quite as well.

Agreed, capif sounds weird. I find camif a bit confusing though, because
Samsung S3C has a camera interface that is actually called "CAMIF".

how about simply "capture" ?

That sounds good to me.


done.




This should really be handled by v4l2_pipeline_pm_use.

I thought about this earlier, but v4l2_pipeline_pm_use() seems to be
doing some other stuff that bothered me, at least that's what I remember.
I will revisit this.

I have used it with a tc358743 -> mipi-csi2 pipeline, it didn't cause
any problems. It would be better to reuse and, if necessary, fix the
existing infrastructure where available.

I tried this API, by switching to v4l2_pipeline_pm_use() in camif
open/release,
and switched to v4l2_pipeline_link_notify() instead of
imx_media_link_notify()
in the media driver's media_device_ops.

This API assumes the video device has an open file handle while the media
links are being established. This doesn't work for me, I want to be able to
establish the links using 'media-ctl -l', and that won't work unless
there is an
open file handle on the video capture device node.

Also, I looked into calling v4l2_pipeline_pm_use() during
imx_media_link_notify(),
instead of imx_media_pipeline_set_power(). Again there are problems with
that.

First, v4l2_pipeline_pm_use() acquires the graph mutex, so it can't be
called inside
link_notify which already acquires that lock. The header for this
function also
clearly states it should only be called in open/release.

So why not call it in open/release then?


er, see above (?)




Second, ignoring the above locking issue for a moment,
v4l2_pipeline_pm_use()
will call s_power on the sensor _first_, then the mipi csi-2 s_power,
when executing
media-ctl -l '"ov5640 1-003c":0 -> "imx6-mipi-csi2":0[1]'. Which is the
wrong order.
In my version which enforces the correct power on order, the mipi csi-2
s_power
is called first in that link setup, followed by the sensor.

I don't understand why you want to power up subdevs as soon as the links
are established.


Because that is the precedence, all other media drivers do pipeline
power on/off at link_notify. And v4l2_pipeline_link_notify() was written
as a link_notify method.


  Shouldn't that rather be done for all subdevices in the
pipeline when the corresponding capture device is opened?


that won't work. There's no guarantee the links will be established
at capture device open time.


It seems to me that powering up the pipeline should be the last step
before userspace actually starts the capture.


Well, I'm ok with moving pipeline power on/off to start/stop streaming.
I would actually prefer to do it then, I only chose at link_notify because
of precedence. I'll look into it.

Steve

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 16/24] media: Add i.MX media core driver

2017-01-23 Thread Philipp Zabel 
Hi Steve,

On Sun, 2017-01-22 at 18:31 -0800, Steve Longerbeam wrote:
> 
> On 01/16/2017 05:47 AM, Philipp Zabel wrote:
> > On Sat, 2017-01-14 at 14:46 -0800, Steve Longerbeam wrote:
> > [...]
>  +Unprocessed Video Capture:
>  +--
>  +
>  +Send frames directly from sensor to camera interface, with no
>  +conversions:
>  +
>  +-> ipu_smfc -> camif
> >>> I'd call this capture interface, this is not just for cameras. Or maybe
> >>> idmac if you want to mirror hardware names?
> >> Camif is so named because it is the V4L2 user interface for video
> >> capture. I suppose it could be named "capif", but that doesn't role
> >> off the tongue quite as well.
> > Agreed, capif sounds weird. I find camif a bit confusing though, because
> > Samsung S3C has a camera interface that is actually called "CAMIF".
> 
> how about simply "capture" ?

That sounds good to me.

[...]
> > Chapter 37.4.2.3 "FCW & FCR - Format converter write and read" in the
> > IDMAC chapter states that all internal submodules only work on 8-bit per
> > component formats with four components: YUVA or RGBA.
> 
> Right, the "direct" IPU internal (that do not transfer buffers to/from
> memory via IDMAC channels) should only allow the IPU internal
> formats YUVA and RGBA. I get you now.
> 
> The "direct" pads now only accept MEDIA_BUS_FMT_AYUV8_1X32 and
> MEDIA_BUS_FMT_ARGB_1X32.
> 
> Those pads are:
> 
> ipu_csi:1
> ipu_vdic:1
> ipu_ic_prp:0
> ipu_ic_prp:1
> ipu_ic_prpenc:0
> ipu_ic_prpenc:1
> ipu_ic_prpvf:0
> ipu_ic_prpvf:1

Yes, that is what I meant. The csi:1 can then be extended to support
additional expanded/packed/raw formats for the SMFC->memory path.

> >> There does not appear to be support in the FSU for linking a write channel
> >> to the VDIC read channels (8, 9, 10) according to VDI_SRC_SEL field. There
> >> is support for the direct link from CSI (which I am using), but that's
> >> not an
> >> IDMAC channel link.
> >>
> >> There is a PRP_SRC_SEL field, with linking from IDMAC (SMFC) channels
> >> 0..2 (and 3? it's not clear, and not clear whether this includes channel 
> >> 1).
> > As I read it, that is 0 and 2 only, no idea why. But since there are
> > only 2 CSIs, that shouldn't be a problem.
> 
> ipu_csi1 is now transferring on IDMAC/SMFC channel 2 (ipu_csi0 still
> at channel 0).

Ok.

>  +static const u32 power_off_seq[] = {
>  +IMX_MEDIA_GRP_ID_IC_PP,
>  +IMX_MEDIA_GRP_ID_IC_PRPVF,
>  +IMX_MEDIA_GRP_ID_IC_PRPENC,
>  +IMX_MEDIA_GRP_ID_SMFC,
>  +IMX_MEDIA_GRP_ID_CSI,
>  +IMX_MEDIA_GRP_ID_VIDMUX,
>  +IMX_MEDIA_GRP_ID_SENSOR,
>  +IMX_MEDIA_GRP_ID_CSI2,
>  +};
> >>> This seems somewhat arbitrary. Why is a power sequence needed?
> >> The CSI-2 receiver must be powered up before the sensor, that's the
> >> only requirement IIRC. The others have no s_power requirement. So I
> >> can probably change this to power up in the frontend -> backend order
> >> (IC_PP to sensor). And vice-versa for power off.
> > Yes, I think that should work (see below).
> 
> Actually there are problems using this, see below.
[...]
>  +EXPORT_SYMBOL_GPL(imx_media_pipeline_set_power);
> >>> This should really be handled by v4l2_pipeline_pm_use.
> >> I thought about this earlier, but v4l2_pipeline_pm_use() seems to be
> >> doing some other stuff that bothered me, at least that's what I remember.
> >> I will revisit this.
> > I have used it with a tc358743 -> mipi-csi2 pipeline, it didn't cause
> > any problems. It would be better to reuse and, if necessary, fix the
> > existing infrastructure where available.
> 
> I tried this API, by switching to v4l2_pipeline_pm_use() in camif 
> open/release,
> and switched to v4l2_pipeline_link_notify() instead of 
> imx_media_link_notify()
> in the media driver's media_device_ops.
> 
> This API assumes the video device has an open file handle while the media
> links are being established. This doesn't work for me, I want to be able to
> establish the links using 'media-ctl -l', and that won't work unless 
> there is an
> open file handle on the video capture device node.
> 
> Also, I looked into calling v4l2_pipeline_pm_use() during 
> imx_media_link_notify(),
> instead of imx_media_pipeline_set_power(). Again there are problems with 
> that.
> 
> First, v4l2_pipeline_pm_use() acquires the graph mutex, so it can't be 
> called inside
> link_notify which already acquires that lock. The header for this 
> function also
> clearly states it should only be called in open/release.

So why not call it in open/release then?

> Second, ignoring the above locking issue for a moment, 
> v4l2_pipeline_pm_use()
> will call s_power on the sensor _first_, then the mipi csi-2 s_power, 
> when executing
> media-ctl -l '"ov5640 1-003c":0 -> "imx6-mipi-csi2":0[1]'. Which is the 
> wrong order.
> In my version which enforces the correct power on order, the mipi csi-2 
> s_power
> is

Re: [PATCH v3 16/24] media: Add i.MX media core driver

2017-01-22 Thread Steve Longerbeam 



On 01/16/2017 05:47 AM, Philipp Zabel wrote:

On Sat, 2017-01-14 at 14:46 -0800, Steve Longerbeam wrote:
[...]

+Unprocessed Video Capture:
+--
+
+Send frames directly from sensor to camera interface, with no
+conversions:
+
+-> ipu_smfc -> camif

I'd call this capture interface, this is not just for cameras. Or maybe
idmac if you want to mirror hardware names?

Camif is so named because it is the V4L2 user interface for video
capture. I suppose it could be named "capif", but that doesn't role
off the tongue quite as well.

Agreed, capif sounds weird. I find camif a bit confusing though, because
Samsung S3C has a camera interface that is actually called "CAMIF".


how about simply "capture" ?





+   media-ctl -V "\"camif1\":0 [fmt:UYVY2X8/640x480]"

I agree this looks very intuitive, but technically correct for the
csi1:1 and camif1:0 pads would be a 32-bit YUV format.
(MEDIA_BUS_FMT_YUV8_1X32_PADLO doesn't exist yet).

I think it would be better to use the correct format
I'm not sure I follow you here.

The ov5640 sends UYVY2X8 on the wire, so pads "ov5640_mipi 1-0040":0
up to "ipu1_csi1":0 are correct. But the CSI writes 32-bit YUV values
into the SMFC, so the CSI output pad and the IDMAC input pad should have
a YUV8_1X32 format.

Chapter 37.4.2.3 "FCW & FCR - Format converter write and read" in the
IDMAC chapter states that all internal submodules only work on 8-bit per
component formats with four components: YUVA or RGBA.


Right, the "direct" IPU internal (that do not transfer buffers to/from
memory via IDMAC channels) should only allow the IPU internal
formats YUVA and RGBA. I get you now.

The "direct" pads now only accept MEDIA_BUS_FMT_AYUV8_1X32 and
MEDIA_BUS_FMT_ARGB_1X32.

Those pads are:

ipu_csi:1
ipu_vdic:1
ipu_ic_prp:0
ipu_ic_prp:1
ipu_ic_prpenc:0
ipu_ic_prpenc:1
ipu_ic_prpvf:0
ipu_ic_prpvf:1






There does not appear to be support in the FSU for linking a write channel
to the VDIC read channels (8, 9, 10) according to VDI_SRC_SEL field. There
is support for the direct link from CSI (which I am using), but that's
not an
IDMAC channel link.

There is a PRP_SRC_SEL field, with linking from IDMAC (SMFC) channels
0..2 (and 3? it's not clear, and not clear whether this includes channel 1).

As I read it, that is 0 and 2 only, no idea why. But since there are
only 2 CSIs, that shouldn't be a problem.


ipu_csi1 is now transferring on IDMAC/SMFC channel 2 (ipu_csi0 still
at channel 0).




+static const u32 power_off_seq[] = {
+   IMX_MEDIA_GRP_ID_IC_PP,
+   IMX_MEDIA_GRP_ID_IC_PRPVF,
+   IMX_MEDIA_GRP_ID_IC_PRPENC,
+   IMX_MEDIA_GRP_ID_SMFC,
+   IMX_MEDIA_GRP_ID_CSI,
+   IMX_MEDIA_GRP_ID_VIDMUX,
+   IMX_MEDIA_GRP_ID_SENSOR,
+   IMX_MEDIA_GRP_ID_CSI2,
+};

This seems somewhat arbitrary. Why is a power sequence needed?

The CSI-2 receiver must be powered up before the sensor, that's the
only requirement IIRC. The others have no s_power requirement. So I
can probably change this to power up in the frontend -> backend order
(IC_PP to sensor). And vice-versa for power off.

Yes, I think that should work (see below).


Actually there are problems using this, see below.


[...]

+/*
+ * Turn current pipeline power on/off starting from start_entity.
+ * Must be called with mdev->graph_mutex held.
+ */
+int imx_media_pipeline_set_power(struct imx_media_dev *imxmd,
+struct media_entity_graph *graph,
+struct media_entity *start_entity, bool on)
+{
+   struct media_entity *entity;
+   struct v4l2_subdev *sd;
+   int i, ret = 0;
+   u32 id;
+
+   for (i = 0; i < NUM_POWER_ENTITIES; i++) {
+   id = on ? power_on_seq[i] : power_off_seq[i];
+   entity = find_pipeline_entity(imxmd, graph, start_entity, id);
+   if (!entity)
+   continue;
+
+   sd = media_entity_to_v4l2_subdev(entity);
+
+   ret = v4l2_subdev_call(sd, core, s_power, on);
+   if (ret && ret != -ENOIOCTLCMD)
+   break;
+   }
+
+   return (ret && ret != -ENOIOCTLCMD) ? ret : 0;
+}
+EXPORT_SYMBOL_GPL(imx_media_pipeline_set_power);

This should really be handled by v4l2_pipeline_pm_use.

I thought about this earlier, but v4l2_pipeline_pm_use() seems to be
doing some other stuff that bothered me, at least that's what I remember.
I will revisit this.

I have used it with a tc358743 -> mipi-csi2 pipeline, it didn't cause
any problems. It would be better to reuse and, if necessary, fix the
existing infrastructure where available.


I tried this API, by switching to v4l2_pipeline_pm_use() in camif 
open/release,
and switched to v4l2_pipeline_link_notify() instead of 
imx_media_link_notify()

in the media driver's media_device_ops.

This API assumes the video device has an open file handle while the media
links are being established. This doesn't work for me, I want to be able to
establish

Re: [PATCH v3 16/24] media: Add i.MX media core driver

2017-01-18 Thread Steve Longerbeam 



On 01/14/2017 02:42 PM, Steve Longerbeam wrote:



+/* parse inputs property from a sensor node */
+static void of_parse_sensor_inputs(struct imx_media_dev *imxmd,
+  struct imx_media_subdev *sensor,
+  struct device_node *sensor_np)
+{
+   struct imx_media_sensor_input *sinput = >input;
+   int ret, i;
+
+   for (i = 0; i < IMX_MEDIA_MAX_SENSOR_INPUTS; i++) {
+   const char *input_name;
+   u32 val;
+
+   ret = of_property_read_u32_index(sensor_np, "inputs", i, );
+   if (ret)
+   break;
+
+   sinput->value[i] = val;
+
+   ret = of_property_read_string_index(sensor_np, "input-names",
+   i, _name);
+   /*
+* if input-names not provided, they will be set using
+* the subdev name once the sensor is known during
+* async bind
+*/
+   if (!ret)
+   strncpy(sinput->name[i], input_name,
+   sizeof(sinput->name[i]));
+   }
+
+   sinput->num = i;
+
+   /* if no inputs provided just assume a single input */
+   if (sinput->num == 0)
+   sinput->num = 1;
+}

This should be parsed by the sensor driver, not imx-media.


you're probably right. I'll submit a patch for adv7180.c.


Actually, the problem here is that this parses an input routing value to
pass to s_routing, and an input name string. There would need to be
another subdev callback, maybe enum_imput, that would return this
information for the bridge driver, if this info were to be parsed and
maintained by the sensor.

But this info should really be known and parsed by the bridge anyway,
because as the header for s_routing states,

"An i2c device shouldn't know about whether an input pin is connected
 to a Composite connector, because on another board or platform it
 might be connected to something else entirely. The calling driver is
 responsible for mapping a user-level input to the right pins on the i2c
 device."

Steve


--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 16/24] media: Add i.MX media core driver

2017-01-16 Thread Philipp Zabel 
On Sat, 2017-01-14 at 14:46 -0800, Steve Longerbeam wrote:
[...]
> >> +Unprocessed Video Capture:
> >> +--
> >> +
> >> +Send frames directly from sensor to camera interface, with no
> >> +conversions:
> >> +
> >> +-> ipu_smfc -> camif
> > I'd call this capture interface, this is not just for cameras. Or maybe
> > idmac if you want to mirror hardware names?
> 
> Camif is so named because it is the V4L2 user interface for video
> capture. I suppose it could be named "capif", but that doesn't role
> off the tongue quite as well.

Agreed, capif sounds weird. I find camif a bit confusing though, because
Samsung S3C has a camera interface that is actually called "CAMIF".

> >> +Note the ipu_smfc can do pixel reordering within the same colorspace.
> > That isn't a feature of the SMFC, but of the IDMAC (FCW & FCR).
> 
> yes, the doc is re-worded to make that more clear.
> 
> >> +For example, its sink pad can take UYVY2X8, but its source pad can
> >> +output YUYV2X8.
> > I don't think this is correct. Re-reading "37.4.3.7 Packing to memory"
> > in the CSI chapter, for 8-bit per component data, the internal format
> > between CSI, SMFC, and IDMAC is always some 32-bit RGBx/YUVx variant
> > (or "bayer/generic data"). In either case, the internal format does not
> > change along the way.
> 
> these are pixels in memory buffers, not the IPU internal formats.

As long as we are talking about the CSI -> SMFC -> IDMAC path, these
should be IPU internal formats. How else would one choose between 8-bit
companded RGB, and 16-bit expanded RGB for a 10-bit per component input
signal? This is the same issue as in the next comment.

> >> +   media-ctl -V "\"camif0\":0 [fmt:UYVY2X8/640x480]"
> >> +   media-ctl -V "\"camif0\":1 [fmt:UYVY2X8/640x480]"
> >> +   # Configure pads for OV5640 pipeline
> >> +   media-ctl -V "\"ov5640_mipi 1-0040\":0 [fmt:UYVY2X8/640x480]"
> >> +   media-ctl -V "\"imx-mipi-csi2\":0 [fmt:UYVY2X8/640x480]"
> >> +   media-ctl -V "\"imx-mipi-csi2\":2 [fmt:UYVY2X8/640x480]"
> >> +   media-ctl -V "\"ipu1_csi1\":0 [fmt:UYVY2X8/640x480]"
> >> +   media-ctl -V "\"ipu1_csi1\":1 [fmt:UYVY2X8/640x480]"
> > [...]
> >> +   media-ctl -V "\"camif1\":0 [fmt:UYVY2X8/640x480]"
> > I agree this looks very intuitive, but technically correct for the
> > csi1:1 and camif1:0 pads would be a 32-bit YUV format.
> > (MEDIA_BUS_FMT_YUV8_1X32_PADLO doesn't exist yet).
> >
> > I think it would be better to use the correct format
> 
> I'm not sure I follow you here.

The ov5640 sends UYVY2X8 on the wire, so pads "ov5640_mipi 1-0040":0
up to "ipu1_csi1":0 are correct. But the CSI writes 32-bit YUV values
into the SMFC, so the CSI output pad and the IDMAC input pad should have
a YUV8_1X32 format.

Chapter 37.4.2.3 "FCW & FCR - Format converter write and read" in the
IDMAC chapter states that all internal submodules only work on 8-bit per
component formats with four components: YUVA or RGBA.

> > [...]
> > Is this a whole software buffer queue implementation? I thought the
> > whole point of putting the custom mem2mem framework into the capture
> > driver was to use the hardware FSU channel linking?
> 
>   see below.
> 
> > What is the purpose of this if the sink should be triggered by the FSU?
> 
> Ok, here is where I need to make an admission.
> 
> The only FSU links I have attempted (and which currently have entries
> in the fsu_link_info[] table), are the enc/vf/pp --> IRT links for rotation.

Which are not described as media entity links because the rotation units
do not have separate media entities. So me arguing against handling
mem2mem chaining via media entity links doesn't concern these implicit
links.

> There does not appear to be support in the FSU for linking a write channel
> to the VDIC read channels (8, 9, 10) according to VDI_SRC_SEL field. There
> is support for the direct link from CSI (which I am using), but that's 
> not an
> IDMAC channel link.
> 
> There is a PRP_SRC_SEL field, with linking from IDMAC (SMFC) channels
> 0..2 (and 3? it's not clear, and not clear whether this includes channel 1).

As I read it, that is 0 and 2 only, no idea why. But since there are
only 2 CSIs, that shouldn't be a problem.

> But I think this links to channel 12, and not to channels 8,9,10 to the 
> VDIC.
> Or will it? It's worth experimenting. It would have helped if FSL listed 
> which
> IDMAC channels these FSU links correspond to, instead of making us guess
> at it.

I would have assumed that the FSU triggering only works on 1:1 channels
and the VDIC with its three input channels is different. But then
there's the alleged VDOA link to ch8/9/10 ro ch9, depending on
VDI_MOT_SEL.

This makes me more convinced that the CSI -> VDIC link should only
describe the direct path (real-time mode, single field).

> In any event, the docs are not clear enough to implement a real FSU
> link to the VDIC read channels, if it's even possible. And trying to get
> programming help from FSL can be difficult, and no coding

Re: [PATCH v3 16/24] media: Add i.MX media core driver

2017-01-14 Thread Steve Longerbeam 

(sorry sending again w/o html)


On 01/13/2017 07:20 AM, Philipp Zabel wrote:

Am Freitag, den 06.01.2017, 18:11 -0800 schrieb Steve Longerbeam:

+For image capture, the IPU contains the following internal subunits:
+
+- Image DMA Controller (IDMAC)
+- Camera Serial Interface (CSI)
+- Image Converter (IC)
+- Sensor Multi-FIFO Controller (SMFC)
+- Image Rotator (IRT)
+- Video De-Interlace Controller (VDIC)

Nitpick: Video De-Interlacing or Combining Block (VDIC)


done.


+
+The IDMAC is the DMA controller for transfer of image frames to and from
+memory. Various dedicated DMA channels exist for both video capture and
+display paths.
+
+The CSI is the frontend capture unit that interfaces directly with
+capture sensors over Parallel, BT.656/1120, and MIPI CSI-2 busses.
+
+The IC handles color-space conversion, resizing, and rotation
+operations.

And horizontal flipping.


done.



There are three independent "tasks" within the IC that can
+carry out conversions concurrently: pre-processing encoding,
+pre-processing preview, and post-processing.

s/preview/viewfinder/ seems to be the commonly used name.


replaced everywhere in the doc.


This paragraph could mention that a single hardware unit is used
transparently time multiplexed by the three tasks at different
granularity for the downsizing, main processing, and rotation sections.
The downscale unit switches between tasks at 8-pixel burst granularity,
the main processing unit at line granularity. The rotation units switch
only at frame granularity.


I've added that info.


+The SMFC is composed of four independent channels that each can transfer
+captured frames from sensors directly to memory concurrently.
+
+The IRT carries out 90 and 270 degree image rotation operations.

... on 8x8 pixel blocks, supported by the IDMAC which handles block
transfers, block reordering, and vertical flipping.


done.


+The VDIC handles the conversion of interlaced video to progressive, with
+support for different motion compensation modes (low, medium, and high
+motion). The deinterlaced output frames from the VDIC can be sent to the
+IC pre-process preview task for further conversions.
+
+In addition to the IPU internal subunits, there are also two units
+outside the IPU that are also involved in video capture on i.MX:
+
+- MIPI CSI-2 Receiver for camera sensors with the MIPI CSI-2 bus
+  interface. This is a Synopsys DesignWare core.
+- A video multiplexer for selecting among multiple sensor inputs to
+  send to a CSI.

Two of them, actually.


done.


+
+- Includes a Frame Interval Monitor (FIM) that can correct vertical sync
+  problems with the ADV718x video decoders. See below for a description
+  of the FIM.

Could this also be used to calculate more precise capture timestamps?


An input capture function could do that, triggered off a VSYNC or FIELD
signal such as on the ADV718x. The FIM is only used to calculate
frame intervals at this point, but its input capture method could be
used to also record more accurate timestamps.



+Capture Pipelines
+-
+
+The following describe the various use-cases supported by the pipelines.
+
+The links shown do not include the frontend sensor, video mux, or mipi
+csi-2 receiver links. This depends on the type of sensor interface
+(parallel or mipi csi-2). So in all cases, these pipelines begin with:
+
+sensor -> ipu_csi_mux -> ipu_csi -> ...
+
+for parallel sensors, or:
+
+sensor -> imx-mipi-csi2 -> (ipu_csi_mux) -> ipu_csi -> ...
+
+for mipi csi-2 sensors. The imx-mipi-csi2 receiver may need to route
+to the video mux (ipu_csi_mux) before sending to the CSI, depending
+on the mipi csi-2 virtual channel, hence ipu_csi_mux is shown in
+parenthesis.
+
+Unprocessed Video Capture:
+--
+
+Send frames directly from sensor to camera interface, with no
+conversions:
+
+-> ipu_smfc -> camif

I'd call this capture interface, this is not just for cameras. Or maybe
idmac if you want to mirror hardware names?


Camif is so named because it is the V4L2 user interface for video
capture. I suppose it could be named "capif", but that doesn't role
off the tongue quite as well.


+Note the ipu_smfc can do pixel reordering within the same colorspace.

That isn't a feature of the SMFC, but of the IDMAC (FCW & FCR).


yes, the doc is re-worded to make that more clear.


+For example, its sink pad can take UYVY2X8, but its source pad can
+output YUYV2X8.

I don't think this is correct. Re-reading "37.4.3.7 Packing to memory"
in the CSI chapter, for 8-bit per component data, the internal format
between CSI, SMFC, and IDMAC is always some 32-bit RGBx/YUVx variant
(or "bayer/generic data"). In either case, the internal format does not
change along the way.


these are pixels in memory buffers, not the IPU internal formats.



+IC Direct Conversions:
+--
+
+This pipeline uses the preprocess encode entity to route frames directly
+from the CSI to the IC (bypassing the SMFC), to carry out

Re: [PATCH v3 16/24] media: Add i.MX media core driver

2017-01-13 Thread Philipp Zabel 
Am Freitag, den 06.01.2017, 18:11 -0800 schrieb Steve Longerbeam:
> Add the core media driver for i.MX SOC.
> 
> Signed-off-by: Steve Longerbeam 
> ---
>  Documentation/media/v4l-drivers/imx.rst   | 443 ++
>  drivers/staging/media/Kconfig |   2 +
>  drivers/staging/media/Makefile|   1 +
>  drivers/staging/media/imx/Kconfig |   8 +
>  drivers/staging/media/imx/Makefile|   6 +
>  drivers/staging/media/imx/TODO|  22 +
>  drivers/staging/media/imx/imx-media-common.c  | 981 
> ++
>  drivers/staging/media/imx/imx-media-dev.c | 486 +++
>  drivers/staging/media/imx/imx-media-fim.c | 471 +++
>  drivers/staging/media/imx/imx-media-internal-sd.c | 457 ++
>  drivers/staging/media/imx/imx-media-of.c  | 289 +++
>  drivers/staging/media/imx/imx-media.h | 310 +++
>  include/media/imx.h   |  15 +
>  include/uapi/linux/v4l2-controls.h|   4 +
>  14 files changed, 3495 insertions(+)
>  create mode 100644 Documentation/media/v4l-drivers/imx.rst
>  create mode 100644 drivers/staging/media/imx/Kconfig
>  create mode 100644 drivers/staging/media/imx/Makefile
>  create mode 100644 drivers/staging/media/imx/TODO
>  create mode 100644 drivers/staging/media/imx/imx-media-common.c
>  create mode 100644 drivers/staging/media/imx/imx-media-dev.c
>  create mode 100644 drivers/staging/media/imx/imx-media-fim.c
>  create mode 100644 drivers/staging/media/imx/imx-media-internal-sd.c
>  create mode 100644 drivers/staging/media/imx/imx-media-of.c
>  create mode 100644 drivers/staging/media/imx/imx-media.h
>  create mode 100644 include/media/imx.h
> 
> diff --git a/Documentation/media/v4l-drivers/imx.rst 
> b/Documentation/media/v4l-drivers/imx.rst
> new file mode 100644
> index 000..87b37b5
> --- /dev/null
> +++ b/Documentation/media/v4l-drivers/imx.rst
> @@ -0,0 +1,443 @@
> +i.MX Video Capture Driver
> +=
> +
> +Introduction
> +
> +
> +The Freescale i.MX5/6 contains an Image Processing Unit (IPU), which
> +handles the flow of image frames to and from capture devices and
> +display devices.
> +
> +For image capture, the IPU contains the following internal subunits:
> +
> +- Image DMA Controller (IDMAC)
> +- Camera Serial Interface (CSI)
> +- Image Converter (IC)
> +- Sensor Multi-FIFO Controller (SMFC)
> +- Image Rotator (IRT)
> +- Video De-Interlace Controller (VDIC)

Nitpick: Video De-Interlacing or Combining Block (VDIC)

> +
> +The IDMAC is the DMA controller for transfer of image frames to and from
> +memory. Various dedicated DMA channels exist for both video capture and
> +display paths.
> +
> +The CSI is the frontend capture unit that interfaces directly with
> +capture sensors over Parallel, BT.656/1120, and MIPI CSI-2 busses.
> +
> +The IC handles color-space conversion, resizing, and rotation
> +operations. 

And horizontal flipping.

> There are three independent "tasks" within the IC that can
> +carry out conversions concurrently: pre-processing encoding,
> +pre-processing preview, and post-processing.

s/preview/viewfinder/ seems to be the commonly used name.

This paragraph could mention that a single hardware unit is used
transparently time multiplexed by the three tasks at different
granularity for the downsizing, main processing, and rotation sections.
The downscale unit switches between tasks at 8-pixel burst granularity,
the main processing unit at line granularity. The rotation units switch
only at frame granularity.

> +The SMFC is composed of four independent channels that each can transfer
> +captured frames from sensors directly to memory concurrently.
> +
> +The IRT carries out 90 and 270 degree image rotation operations.

... on 8x8 pixel blocks, supported by the IDMAC which handles block
transfers, block reordering, and vertical flipping.

> +The VDIC handles the conversion of interlaced video to progressive, with
> +support for different motion compensation modes (low, medium, and high
> +motion). The deinterlaced output frames from the VDIC can be sent to the
> +IC pre-process preview task for further conversions.
> +
> +In addition to the IPU internal subunits, there are also two units
> +outside the IPU that are also involved in video capture on i.MX:
> +
> +- MIPI CSI-2 Receiver for camera sensors with the MIPI CSI-2 bus
> +  interface. This is a Synopsys DesignWare core.
> +- A video multiplexer for selecting among multiple sensor inputs to
> +  send to a CSI.

Two of them, actually.

> +For more info, refer to the latest versions of the i.MX5/6 reference
> +manuals listed under References.
> +
> +
> +Features
> +
> +
> +Some of the features of this driver include:
> +
> +- Many different pipelines can be configured via media controller API,
> +  that correspond to the hardware video capture

[PATCH v3 16/24] media: Add i.MX media core driver

2017-01-06 Thread Steve Longerbeam 
Add the core media driver for i.MX SOC.

Signed-off-by: Steve Longerbeam 
---
 Documentation/media/v4l-drivers/imx.rst   | 443 ++
 drivers/staging/media/Kconfig |   2 +
 drivers/staging/media/Makefile|   1 +
 drivers/staging/media/imx/Kconfig |   8 +
 drivers/staging/media/imx/Makefile|   6 +
 drivers/staging/media/imx/TODO|  22 +
 drivers/staging/media/imx/imx-media-common.c  | 981 ++
 drivers/staging/media/imx/imx-media-dev.c | 486 +++
 drivers/staging/media/imx/imx-media-fim.c | 471 +++
 drivers/staging/media/imx/imx-media-internal-sd.c | 457 ++
 drivers/staging/media/imx/imx-media-of.c  | 289 +++
 drivers/staging/media/imx/imx-media.h | 310 +++
 include/media/imx.h   |  15 +
 include/uapi/linux/v4l2-controls.h|   4 +
 14 files changed, 3495 insertions(+)
 create mode 100644 Documentation/media/v4l-drivers/imx.rst
 create mode 100644 drivers/staging/media/imx/Kconfig
 create mode 100644 drivers/staging/media/imx/Makefile
 create mode 100644 drivers/staging/media/imx/TODO
 create mode 100644 drivers/staging/media/imx/imx-media-common.c
 create mode 100644 drivers/staging/media/imx/imx-media-dev.c
 create mode 100644 drivers/staging/media/imx/imx-media-fim.c
 create mode 100644 drivers/staging/media/imx/imx-media-internal-sd.c
 create mode 100644 drivers/staging/media/imx/imx-media-of.c
 create mode 100644 drivers/staging/media/imx/imx-media.h
 create mode 100644 include/media/imx.h

diff --git a/Documentation/media/v4l-drivers/imx.rst 
b/Documentation/media/v4l-drivers/imx.rst
new file mode 100644
index 000..87b37b5
--- /dev/null
+++ b/Documentation/media/v4l-drivers/imx.rst
@@ -0,0 +1,443 @@
+i.MX Video Capture Driver
+=
+
+Introduction
+
+
+The Freescale i.MX5/6 contains an Image Processing Unit (IPU), which
+handles the flow of image frames to and from capture devices and
+display devices.
+
+For image capture, the IPU contains the following internal subunits:
+
+- Image DMA Controller (IDMAC)
+- Camera Serial Interface (CSI)
+- Image Converter (IC)
+- Sensor Multi-FIFO Controller (SMFC)
+- Image Rotator (IRT)
+- Video De-Interlace Controller (VDIC)
+
+The IDMAC is the DMA controller for transfer of image frames to and from
+memory. Various dedicated DMA channels exist for both video capture and
+display paths.
+
+The CSI is the frontend capture unit that interfaces directly with
+capture sensors over Parallel, BT.656/1120, and MIPI CSI-2 busses.
+
+The IC handles color-space conversion, resizing, and rotation
+operations. There are three independent "tasks" within the IC that can
+carry out conversions concurrently: pre-processing encoding,
+pre-processing preview, and post-processing.
+
+The SMFC is composed of four independent channels that each can transfer
+captured frames from sensors directly to memory concurrently.
+
+The IRT carries out 90 and 270 degree image rotation operations.
+
+The VDIC handles the conversion of interlaced video to progressive, with
+support for different motion compensation modes (low, medium, and high
+motion). The deinterlaced output frames from the VDIC can be sent to the
+IC pre-process preview task for further conversions.
+
+In addition to the IPU internal subunits, there are also two units
+outside the IPU that are also involved in video capture on i.MX:
+
+- MIPI CSI-2 Receiver for camera sensors with the MIPI CSI-2 bus
+  interface. This is a Synopsys DesignWare core.
+- A video multiplexer for selecting among multiple sensor inputs to
+  send to a CSI.
+
+For more info, refer to the latest versions of the i.MX5/6 reference
+manuals listed under References.
+
+
+Features
+
+
+Some of the features of this driver include:
+
+- Many different pipelines can be configured via media controller API,
+  that correspond to the hardware video capture pipelines supported in
+  the i.MX.
+
+- Supports parallel, BT.565, and MIPI CSI-2 interfaces.
+
+- Up to four concurrent sensor acquisitions, by configuring each
+  sensor's pipeline using independent entities. This is currently
+  demonstrated with the SabreSD and SabreLite reference boards with
+  independent OV5642 and MIPI CSI-2 OV5640 sensor modules.
+
+- Scaling, color-space conversion, and image rotation via IC task
+  subdevs.
+
+- Many pixel formats supported (RGB, packed and planar YUV, partial
+  planar YUV).
+
+- The IC pre-process preview subdev supports motion compensated
+  de-interlacing using the VDIC, with three motion compensation modes:
+  low, medium, and high motion. The mode is specified with a custom
+  control. Pipelines are defined that allow sending frames to the
+  preview subdev directly from the CSI or from the SMFC.
+
+- Includes a Frame Interval Monitor (FIM) that can correct