Hi Steve,

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

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

quoted

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

quoted

quoted

quoted

+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.

[...]

quoted

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_ARGB8888_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.

quoted

quoted

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.

quoted

quoted

quoted

quoted

+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.

[...]

quoted

quoted

quoted

quoted

+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 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.

[...]

quoted

quoted

quoted

quoted

+	/* there must be at least one CSI in first IPU */

Why?

Well yeah, imx-media doesn't necessarily need a CSI if things
like the VDIC or post-processor are being used by an output
overlay pipeline, for example. I'll fix this.

I haven't even thought that far, but there could be boards with only a
parallel sensor connected to IPU2 CSI1 and IPU1 disabled for power
saving reasons.

done! A very simple change to imx_media_add_internal_subdevs(),
and now no CSI's are necessary, but the remaining IPU internal entities
are loaded and linked just fine without them, so for example with no
CSI's in IPU2, the VDIC entity in IPU2 is still present in the graph and
could still be used in the future by a mem2mem device for instance.

Excellent, thanks.

regards
Philipp