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

[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