Re: [PATCH v3 1/1] v4l: event: Add subscription to list before calling "add" operation

[Dave Stevenson]

On Tue, 6 Nov 2018 at 08:00, Sakari Ailus  wrote:
>
> Patch ad608fbcf166 changed how events were subscribed to address an issue
> elsewhere. As a side effect of that change, the "add" callback was called
> before the event subscription was added to the list of subscribed events,
> causing the first event queued by the add callback (and possibly other
> events arriving soon afterwards) to be lost.
>
> Fix this by adding the subscription to the list before calling the "add"
> callback, and clean up afterwards if that fails.
>
> Fixes: ad608fbcf166 ("media: v4l: event: Prevent freeing event subscriptions 
> while accessed")
>
> Reported-by: Dave Stevenson 
> Signed-off-by: Sakari Ailus 

Tested-By: Dave Stevenson 

Tested with 3 bcm2835 drivers (camera driver in staging, CSI2
receiver, and codec M2M driver) via v4l2-compliance on 4.19.0. All 3
failed in the same way prior to this patch.

> ---
> since v2:
>
> - More accurate commit message. No other changes.
>
>  drivers/media/v4l2-core/v4l2-event.c | 43 
> 
>  1 file changed, 24 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/v4l2-event.c 
> b/drivers/media/v4l2-core/v4l2-event.c
> index a3ef1f50a4b3..481e3c65cf97 100644
> --- a/drivers/media/v4l2-core/v4l2-event.c
> +++ b/drivers/media/v4l2-core/v4l2-event.c
> @@ -193,6 +193,22 @@ int v4l2_event_pending(struct v4l2_fh *fh)
>  }
>  EXPORT_SYMBOL_GPL(v4l2_event_pending);
>
> +static void __v4l2_event_unsubscribe(struct v4l2_subscribed_event *sev)
> +{
> +   struct v4l2_fh *fh = sev->fh;
> +   unsigned int i;
> +
> +   lockdep_assert_held(>subscribe_lock);
> +   assert_spin_locked(>vdev->fh_lock);
> +
> +   /* Remove any pending events for this subscription */
> +   for (i = 0; i < sev->in_use; i++) {
> +   list_del(>events[sev_pos(sev, i)].list);
> +   fh->navailable--;
> +   }
> +   list_del(>list);
> +}
> +
>  int v4l2_event_subscribe(struct v4l2_fh *fh,
>  const struct v4l2_event_subscription *sub, unsigned 
> elems,
>  const struct v4l2_subscribed_event_ops *ops)
> @@ -224,27 +240,23 @@ int v4l2_event_subscribe(struct v4l2_fh *fh,
>
> spin_lock_irqsave(>vdev->fh_lock, flags);
> found_ev = v4l2_event_subscribed(fh, sub->type, sub->id);
> +   if (!found_ev)
> +   list_add(>list, >subscribed);
> spin_unlock_irqrestore(>vdev->fh_lock, flags);
>
> if (found_ev) {
> /* Already listening */
> kvfree(sev);
> -   goto out_unlock;
> -   }
> -
> -   if (sev->ops && sev->ops->add) {
> +   } else if (sev->ops && sev->ops->add) {
> ret = sev->ops->add(sev, elems);
> if (ret) {
> +   spin_lock_irqsave(>vdev->fh_lock, flags);
> +   __v4l2_event_unsubscribe(sev);
> +   spin_unlock_irqrestore(>vdev->fh_lock, flags);
> kvfree(sev);
> -   goto out_unlock;
> }
> }
>
> -   spin_lock_irqsave(>vdev->fh_lock, flags);
> -   list_add(>list, >subscribed);
> -   spin_unlock_irqrestore(>vdev->fh_lock, flags);
> -
> -out_unlock:
> mutex_unlock(>subscribe_lock);
>
> return ret;
> @@ -279,7 +291,6 @@ int v4l2_event_unsubscribe(struct v4l2_fh *fh,
>  {
> struct v4l2_subscribed_event *sev;
> unsigned long flags;
> -   int i;
>
> if (sub->type == V4L2_EVENT_ALL) {
> v4l2_event_unsubscribe_all(fh);
> @@ -291,14 +302,8 @@ int v4l2_event_unsubscribe(struct v4l2_fh *fh,
> spin_lock_irqsave(>vdev->fh_lock, flags);
>
> sev = v4l2_event_subscribed(fh, sub->type, sub->id);
> -   if (sev != NULL) {
> -   /* Remove any pending events for this subscription */
> -   for (i = 0; i < sev->in_use; i++) {
> -   list_del(>events[sev_pos(sev, i)].list);
> -   fh->navailable--;
> -   }
> -   list_del(>list);
> -   }
> +   if (sev != NULL)
> +   __v4l2_event_unsubscribe(sev);
>
> spin_unlock_irqrestore(>vdev->fh_lock, flags);
>
> --
> 2.11.0
>

Re: [PATCH 1/1] v4l: event: Add subscription to list before calling "add" operation

[Dave Stevenson]

Hi Sakari

On Mon, 5 Nov 2018 at 15:46, Sakari Ailus  wrote:
>
> Patch ad608fbcf166 changed how events were subscribed to address an issue
> elsewhere. As a side effect of that change, the "add" callback was called
> before the event subscription was added to the list of subscribed events,
> causing the first event (and possibly other events arriving soon
> afterwards) to be lost.
>
> Fix this by adding the subscription to the list before calling the "add"
> callback, and clean up afterwards if that fails.
>
> Fixes: ad608fbcf166 ("media: v4l: event: Prevent freeing event subscriptions 
> while accessed")
>
> Reported-by: Dave Stevenson 
> Signed-off-by: Sakari Ailus 
> ---
> Hi Dave, Hans,
>
> This should address the issue.
>
> The functions can (and probably should) be re-arranged later but let's get
> the fix right first.
>
> I haven't tested this using vivid yet as it crashes where I was testing
> it. I'll see later if it works elsewhere but I'm sending the patch for
> review in the meantime.

Thanks. That seems to fix the failure for my 3 drivers.

For what it's worth:
Tested-by: Dave Stevenson 

I haven't tested the failure path, but it looks sane.

>  drivers/media/v4l2-core/v4l2-event.c | 39 
> +++-
>  1 file changed, 25 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/v4l2-event.c 
> b/drivers/media/v4l2-core/v4l2-event.c
> index a3ef1f50a4b3..2b6651aeb89b 100644
> --- a/drivers/media/v4l2-core/v4l2-event.c
> +++ b/drivers/media/v4l2-core/v4l2-event.c
> @@ -193,6 +193,22 @@ int v4l2_event_pending(struct v4l2_fh *fh)
>  }
>  EXPORT_SYMBOL_GPL(v4l2_event_pending);
>
> +static void __v4l2_event_unsubscribe(struct v4l2_subscribed_event *sev)
> +{
> +   struct v4l2_fh *fh = sev->fh;
> +   unsigned int i;
> +
> +   lockdep_assert_held(>subscribe_lock);
> +   assert_spin_locked(>vdev->fh_lock);
> +
> +   /* Remove any pending events for this subscription */
> +   for (i = 0; i < sev->in_use; i++) {
> +   list_del(>events[sev_pos(sev, i)].list);
> +   fh->navailable--;
> +   }
> +   list_del(>list);
> +}
> +
>  int v4l2_event_subscribe(struct v4l2_fh *fh,
>  const struct v4l2_event_subscription *sub, unsigned 
> elems,
>  const struct v4l2_subscribed_event_ops *ops)
> @@ -232,18 +248,20 @@ int v4l2_event_subscribe(struct v4l2_fh *fh,
> goto out_unlock;
> }
>
> +   spin_lock_irqsave(>vdev->fh_lock, flags);
> +   list_add(>list, >subscribed);
> +   spin_unlock_irqrestore(>vdev->fh_lock, flags);
> +
> if (sev->ops && sev->ops->add) {
> ret = sev->ops->add(sev, elems);
> if (ret) {
> +   spin_lock_irqsave(>vdev->fh_lock, flags);
> +   __v4l2_event_unsubscribe(sev);
> +   spin_unlock_irqrestore(>vdev->fh_lock, flags);
> kvfree(sev);
> -   goto out_unlock;
> }
> }
>
> -   spin_lock_irqsave(>vdev->fh_lock, flags);
> -   list_add(>list, >subscribed);
> -   spin_unlock_irqrestore(>vdev->fh_lock, flags);
> -
>  out_unlock:
> mutex_unlock(>subscribe_lock);
>
> @@ -279,7 +297,6 @@ int v4l2_event_unsubscribe(struct v4l2_fh *fh,
>  {
> struct v4l2_subscribed_event *sev;
> unsigned long flags;
> -   int i;
>
> if (sub->type == V4L2_EVENT_ALL) {
> v4l2_event_unsubscribe_all(fh);
> @@ -291,14 +308,8 @@ int v4l2_event_unsubscribe(struct v4l2_fh *fh,
> spin_lock_irqsave(>vdev->fh_lock, flags);
>
> sev = v4l2_event_subscribed(fh, sub->type, sub->id);
> -   if (sev != NULL) {
> -   /* Remove any pending events for this subscription */
> -   for (i = 0; i < sev->in_use; i++) {
> -   list_del(>events[sev_pos(sev, i)].list);
> -   fh->navailable--;
> -   }
> -   list_del(>list);
> -   }
> +   if (sev != NULL)
> +   __v4l2_event_unsubscribe(sev);
>
> spin_unlock_irqrestore(>vdev->fh_lock, flags);
>
> --
> 2.11.0
>

Re: VIDIOC_SUBSCRIBE_EVENT for V4L2_EVENT_CTRL regression?

[Dave Stevenson]

Hi Hans

On Mon, 5 Nov 2018 at 13:18, Hans Verkuil  wrote:
>
> On 11/05/2018 01:21 PM, Dave Stevenson wrote:
> > Hi All
> >
> > I'm testing with 4.19 and finding that testEvents in v4l2-compliance
> > is failing with ""failed to find event for control '%s' type %u", ie
> > it hasn't got the event for the inital values. This is with the
> > various BCM2835 drivers that I'm involved with.
> >
> > Having looked at the v4l2-core history I tried reverting "ad608fb
> > media: v4l: event: Prevent freeing event subscriptions while
> > accessed". The test passes again.
> >
> > Enabling all logging, and adding a couple of logging messages at the
> > beginning and end of v4l2_ctrl_add_event and __v4l2_event_queue_fh
> > error path, I get the following logs:
> >
> > [   90.62] v4l2_ctrl_add_event: ctrl a2b86fa8 "User Controls" type
> > 6, flags 0001
> > [   90.630002] video0: VIDIOC_SUBSCRIBE_EVENT: type=0x3, id=0x980001, 
> > flags=0x1
> > [   91.630166] videodev: v4l2_poll: video0: poll: 
> > [   91.630311] videodev: v4l2_poll: video0: poll: 
> > [   91.630325] video0: VIDIOC_UNSUBSCRIBE_EVENT: type=0x3,
> > id=0x980001, flags=0x1
> > [   91.630396] v4l2_ctrl_add_event: ctrl 8f6fcc61 "Brightness" type 1,
> > flags 0001
> > [   91.630403] __v4l2_event_queue_fh: Not subscribed to event type 3 id 
> > 0x980001
> > [   91.630407] v4l2_ctrl_add_event: ctrl 8f6fcc61 "Brightness" type 1
> > - initial values queued
> > [   91.630409] video0: VIDIOC_SUBSCRIBE_EVENT: type=0x3, id=0x980900, 
> > flags=0x1
> > [   92.630513] videodev: v4l2_poll: video0: poll: 
> > [   92.630660] videodev: v4l2_poll: video0: poll: 
> > [   92.630729] videodev: v4l2_release: video0: release
> >
> > So __v4l2_event_queue_fh is dropping the event as we aren't subscribed
> > at the point that the initial values are queued.
> >
> > Sorry, I don't have any other devices that support subscribing for
> > events to hand (uvcvideo passes the test as it reports unsupported). I
> > don't have a media tree build immediately available either, but I
> > can't see anything related to this in the recent history. I can go
> > down that route if needed.
> > v4l-utils repo was synced today - head at "f9881444e8 cec-compliance:
> > wake-up on Active Source is warn for <2.0"
> >
> > Could someone test on other hardware to confirm that it's not the
> > drivers I'm using? I'm fairly certain it isn't as that patch does call
> > sev->ops->add(sev, elems); before list_add(>list,
> > >subscribed), and that is guaranteed to fail if sev->ops->add
> > immediately queues an event.
>
> Just to pitch in, I got similar issues when I tried to apply that commit
> to our Cisco code base. I've been away for a week and had no time to look
> into the cause, but it really appears that that commit was bad.
>
> Sakari, can you take a look at this?

Thanks for confirming that it's not just me!

Swapping around the order of the list_add and ops->add fixes the
issue, but I'm not clear enough on whether there is a chance for
events to have been raised and need clearing to propose a full patch.
I'm currently running with:
diff --git a/drivers/media/v4l2-core/v4l2-event.c
b/drivers/media/v4l2-core/v4l2-event.c
index a3ef1f5..a997d2e 100644
--- a/drivers/media/v4l2-core/v4l2-event.c
+++ b/drivers/media/v4l2-core/v4l2-event.c
@@ -232,18 +234,20 @@ int v4l2_event_subscribe(struct v4l2_fh *fh,
goto out_unlock;
}

+   spin_lock_irqsave(>vdev->fh_lock, flags);
+   list_add(>list, >subscribed);
+   spin_unlock_irqrestore(>vdev->fh_lock, flags);
+
if (sev->ops && sev->ops->add) {
ret = sev->ops->add(sev, elems);
if (ret) {
+   spin_lock_irqsave(>vdev->fh_lock, flags);
+   list_del(>list);
+   spin_unlock_irqrestore(>vdev->fh_lock, flags);
kvfree(sev);
-   goto out_unlock;
}
}

-   spin_lock_irqsave(>vdev->fh_lock, flags);
-   list_add(>list, >subscribed);
-   spin_unlock_irqrestore(>vdev->fh_lock, flags);
-
 out_unlock:
mutex_unlock(>subscribe_lock);

Is there a need to iterate the sev->events list and free any
potentially pending events there in the ops->add error path?
We still have the subscribe_lock held, so I don't think that it
reintroduces the issue that the patch was fixing of unsubscribing
before subscribed.

This patch has also been merged to stable, so 4.14 is affected as well.

  Dave

VIDIOC_SUBSCRIBE_EVENT for V4L2_EVENT_CTRL regression?

[Dave Stevenson]

Hi All

I'm testing with 4.19 and finding that testEvents in v4l2-compliance
is failing with ""failed to find event for control '%s' type %u", ie
it hasn't got the event for the inital values. This is with the
various BCM2835 drivers that I'm involved with.

Having looked at the v4l2-core history I tried reverting "ad608fb
media: v4l: event: Prevent freeing event subscriptions while
accessed". The test passes again.

Enabling all logging, and adding a couple of logging messages at the
beginning and end of v4l2_ctrl_add_event and __v4l2_event_queue_fh
error path, I get the following logs:

[   90.62] v4l2_ctrl_add_event: ctrl a2b86fa8 "User Controls" type
6, flags 0001
[   90.630002] video0: VIDIOC_SUBSCRIBE_EVENT: type=0x3, id=0x980001, flags=0x1
[   91.630166] videodev: v4l2_poll: video0: poll: 
[   91.630311] videodev: v4l2_poll: video0: poll: 
[   91.630325] video0: VIDIOC_UNSUBSCRIBE_EVENT: type=0x3,
id=0x980001, flags=0x1
[   91.630396] v4l2_ctrl_add_event: ctrl 8f6fcc61 "Brightness" type 1,
flags 0001
[   91.630403] __v4l2_event_queue_fh: Not subscribed to event type 3 id 0x980001
[   91.630407] v4l2_ctrl_add_event: ctrl 8f6fcc61 "Brightness" type 1
- initial values queued
[   91.630409] video0: VIDIOC_SUBSCRIBE_EVENT: type=0x3, id=0x980900, flags=0x1
[   92.630513] videodev: v4l2_poll: video0: poll: 
[   92.630660] videodev: v4l2_poll: video0: poll: 
[   92.630729] videodev: v4l2_release: video0: release

So __v4l2_event_queue_fh is dropping the event as we aren't subscribed
at the point that the initial values are queued.

Sorry, I don't have any other devices that support subscribing for
events to hand (uvcvideo passes the test as it reports unsupported). I
don't have a media tree build immediately available either, but I
can't see anything related to this in the recent history. I can go
down that route if needed.
v4l-utils repo was synced today - head at "f9881444e8 cec-compliance:
wake-up on Active Source is warn for <2.0"

Could someone test on other hardware to confirm that it's not the
drivers I'm using? I'm fairly certain it isn't as that patch does call
sev->ops->add(sev, elems); before list_add(>list,
>subscribed), and that is guaranteed to fail if sev->ops->add
immediately queues an event.

Thanks
  Dave

Re: [RFP] Which V4L2 ioctls could be replaced by better versions?

[Dave Stevenson]

Hi Tomasz

On Mon, 22 Oct 2018 at 04:38, Tomasz Figa  wrote:
>
> Hi Philipp,
>
> On Mon, Oct 22, 2018 at 1:28 AM Philipp Zabel  wrote:
> >
> > On Wed, Oct 03, 2018 at 05:24:39PM +0900, Tomasz Figa wrote:
> > [...]
> > > > Yes, but that would fall in a complete redesign I guess. The buffer
> > > > allocation scheme is very inflexible. You can't have buffers of two
> > > > dimensions allocated at the same time for the same queue. Worst, you
> > > > cannot leave even 1 buffer as your scannout buffer while reallocating
> > > > new buffers, this is not permitted by the framework (in software). As a
> > > > side effect, there is no way to optimize the resolution changes, you
> > > > even have to copy your scannout buffer on the CPU, to free it in order
> > > > to proceed. Resolution changes are thus painfully slow, by design.
> > [...]
> > > Also, I fail to understand the scanout issue. If one exports a vb2
> > > buffer to a DMA-buf and import it to the scanout engine, it can keep
> > > scanning out from it as long as it want, because the DMA-buf will hold
> > > a reference on the buffer, even if it's removed from the vb2 queue.
> >
> > REQBUFS 0 fails if the vb2 buffer is still in use, including from dmabuf
> > attachments: vb2_buffer_in_use checks the num_users memop. The refcount
> > returned by num_users shared between the vmarea handler and dmabuf ops,
> > so any dmabuf attachment counts towards in_use.
>
> Ah, right. I've managed to completely forget about it, since we have a
> downstream patch that we attempted to upstream earlier [1], but didn't
> have a chance to follow up on the comments and there wasn't much
> interest in it in general.
>
> [1] https://lore.kernel.org/patchwork/patch/607853/
>
> Perhaps it would be worth reviving?

There's been recent interest in this from the LibreElec folk as they
are wanting to shift to using V4L2 M2M for codecs on as many platforms
as possible. They'll use it wrapped by FFmpeg, and they're working on
patches to get FFmpeg exporting dmabufs to be consumed by DRM.

Hans had pointed me at your patch, and I've been using it to get
around the issue.
I did start writing the requested docs changes [1], but I'm afraid
upstreaming stuff has been a low priority for me recently. If that
patch suffices, then feel free to pick it up. Otherwise I'll do so
when I get some time (likely to be a fair number of weeks away).

  Dave

[1] 
https://github.com/6by9/linux/commit/09619d9427d9d44ae5e72e0e85e64cb3ea9727da

Re: [RFC] V4L2_PIX_FMT_MJPEG vs V4L2_PIX_FMT_JPEG

[Dave Stevenson]

Hi All,

On Mon, 1 Oct 2018 at 17:32, Ezequiel Garcia  wrote:
>
> Hi Hans,
>
> Thanks for looking into. I remember MJPEG vs. JPEG being a source
> of confusion for me a few years ago, so clarification is greatly
> welcome :-)
>
> On Mon, 2018-10-01 at 15:03 +0300, Laurent Pinchart wrote:
> > Hi Hans,
> >
> > On Monday, 1 October 2018 14:54:29 EEST Hans Verkuil wrote:
> > > On 10/01/2018 01:48 PM, Laurent Pinchart wrote:
> > > > On Monday, 1 October 2018 11:43:04 EEST Hans Verkuil wrote:
> > > > > It turns out that we have both JPEG and Motion-JPEG pixel formats
> > > > > defined.
> > > > >
> > > > > Furthermore, some drivers support one, some the other and some both.
> > > > >
> > > > > These pixelformats both mean the same.
> > > >
> > > > Do they ? I thought MJPEG was JPEG using fixed Huffman tables that were
> > > > not included in the JPEG headers.
> > >
> > > I'm not aware of any difference. If there is one, then it is certainly not
> > > documented.
> >
> > What I can tell for sure is that many UVC devices don't include Huffman 
> > tables
> > in their JPEG headers.
> >
> > > Ezequiel, since you've been working with this recently, do you know 
> > > anything
> > > about this?
> >
> >
>
> JPEG frames must include huffman and quantization tables, as per the standard.
>
> AFAIK, there's no MJPEG specification per-se and vendors specify its own
> way of conveying a Motion JPEG stream.

There is the specfication for MJPEG in Quicktime containers, which
defines the MJPEG-A and MJPEG-B variants [1].
MJPEG-B is not a concatenation of JPEG frames as the framing is
different, so can't really be combined into V4L2_PIX_FMT_JPEG.
Have people encountered devices that produce MJPEG-A or MJPEG-B via
V4L2? I haven't, but I have been forced to support both variants on
decode.

On that thought, whilst capture devices generally don't care, is there
a need to differentiate for M2M codec devices which can support
encoding the variants? Or likewise on M2M decoders that support only
JPEG, how do they tell userspace that they don't support MJPEG-A or
MJPEG-B?

  Dave

[1] https://developer.apple.com/standards/qtff-2001.pdf

> For instance, omiting the huffman table seems to be a vendor thing. Microsoft
> explicitly omits the huffman tables from each frame:
>
> https://www.fileformat.info/format/bmp/spec/b7c72ebab8064da48ae5ed0c053c67a4/view.htm
>
> Others could be following the same things.
>
> Like I mentioned before, Gstreamer always check for missing huffman table
> and adds one if missing. Gstreamer has other quirks for missing markers,
> e.g. deal with a missing EOI:
>
> https://github.com/GStreamer/gst-plugins-good/commit/10ff3c8e14e8fba9e0a5d696dce0bea27de644d7
>
> I think Hans suggestion of settling on JPEG makes sense and it would
> be consistent with Gstreamer. Otherwise, we should specify exactly what we
> understand by MJPEG, but I don't think it's worth it.
>
> Thanks,
> Ezequiel

Re: [ANN] Meeting to discuss improvements to support MC-based cameras on generic apps

[Dave Stevenson]

Hi Laurent

On 19 May 2018 at 08:04, Laurent Pinchart
<laurent.pinch...@ideasonboard.com> wrote:
> Hi Dave,
>
> On Friday, 18 May 2018 18:37:01 EEST Dave Stevenson wrote:
>> On 18 May 2018 at 16:05, Mauro Carvalho Chehab wrote:
>> > Em Fri, 18 May 2018 15:27:24 +0300
>>
>> 
>>
>> >>> There, instead of an USB camera, the hardware is equipped with a
>> >>> MC-based ISP, connected to its camera. Currently, despite having
>> >>> a Kernel driver for it, the camera doesn't work with any
>> >>> userspace application.
>> >>>
>> >>> I'm also aware of other projects that are considering the usage of
>> >>> mc-based devices for non-dedicated hardware.
>> >>
>> >> What are those projects ?
>> >
>> > Well, cheap ARM-based hardware like RPi3 already has this issue: they
>> > have an ISP (or some GPU firmware meant to emulate an ISP). While
>> > those hardware could have multiple sensors, typically they have just
>> > one.
>>
>> Slight hijack, but a closely linked issue for the Pi.
>> The way I understand the issue of V4L2 / MC on Pi is a more
>> fundamental mismatch in architecture. Please correct me if I'm wrong
>> here.
>>
>> The Pi CSI2 receiver peripheral always writes the incoming data to
>> SDRAM, and the ISP is then a memory to memory device.
>>
>> V4L2 subdevices are not dma controllers and therefore have no buffers
>> allocated to them. So to support the full complexity of the pipeline
>> in V4L2 requires that something somewhere would have to be dequeuing
>> the buffers from the CSI receiver V4L2 device and queuing them to the
>> input of a (theoretical) ISP M2M V4L2 device, and returning them once
>> processed. The application only cares about the output of the ISP M2M
>> device.
>
> Regardless of the software stack architecture, something running on the CPU
> has to perform that job. We have decided that that "something" needs to run in
> userspace, to avoid pushing use-case-dependent code to the kernel.
>
> Note that this isn't specific to the RPi. The OMAP3 ISP, while integrating the
> CSI-2 receiver and being able to process data on the fly, can also write the
> raw images to memory and then process them in memory-to-memory mode. This
> feature is used mostly for still image capture to perform pre-processing with
> the CPU (or possibly GPU) on the raw images before processing them in the ISP.
> There's no way we could implement this fully in the kernel.

Sure. I was mainly flagging that having to manage buffers also needs
to be considered in order to make a usable system. Just configuring an
MC pipeline won't solve all the issues.

>> So I guess my question is whether there is a sane mechanism to remove
>> that buffer allocation and handling from the app? Without it we are
>> pretty much forced to hide bigger blobs of functionality to even
>> vaguely fit in with V4L2.
>
> We need a way to remove that from the application, but it won't be pushed down
> to the kernel. These tasks should be handled by a userspace framework,
> transparently for the application. The purpose of this discussion is to decide
> on the design of the framework.

I'm in agreement there, but hadn't seen discussion on buffer
management, only MC configuration.

>> I'm at the point where it shouldn't be a huge amount of work to create
>> at least a basic ISP V4L2 M2M device, but I'm not planning on doing it
>> if it pushes the above buffer handling onto the app because it simply
>> won't get used beyond demo apps. The likes of Cheese, Scratch, etc,
>> just won't do it.
>>
>>
>> To avoid ambiguity, the Pi has a hardware ISP block. There are other
>> SoCs that use either GPU code or a DSP to implement their ISP.
>
> Is that ISP documented publicly ?

Not publicly, and as it's Broadcom's IP we can't release it :-(

What I have working is using the Broadcom MMAL API (very similar to
OpenMAX IL) to wrap the ISP hardware block via the VideoCore firmware.
Currently it has the major controls exposed (black level, digital
gain, white balance, CCMs, lens shading tables) and I'll add
additional controls as time permits or use cases require. Resizing and
format conversion are done based on input and output formats. Defining
stats regions and extracting the resulting stats is still to be done.
Overall it keeps all the implementation details hidden so we don't
break NDAs, but should allow efficient processing. It supports
dma-bufs in and out, so no extra copies of the data should be
required.

I'm currently finishing off a V4L2 M2M wrapper around the MMAL
video_encode and video_decode components, so modifying it to support
the ISP component shouldn't be difficult if there is value in doing
so.
I know it's not the ideal, but our hands are tied.

  Dave

Re: [ANN] Meeting to discuss improvements to support MC-based cameras on generic apps

[Dave Stevenson]

On 18 May 2018 at 16:05, Mauro Carvalho Chehab
 wrote:
> Em Fri, 18 May 2018 15:27:24 +0300

>>
>> > There, instead of an USB camera, the hardware is equipped with a
>> > MC-based ISP, connected to its camera. Currently, despite having
>> > a Kernel driver for it, the camera doesn't work with any
>> > userspace application.
>> >
>> > I'm also aware of other projects that are considering the usage of
>> > mc-based devices for non-dedicated hardware.
>>
>> What are those projects ?
>
> Well, cheap ARM-based hardware like RPi3 already has this issue: they
> have an ISP (or some GPU firmware meant to emulate an ISP). While
> those hardware could have multiple sensors, typically they have just
> one.

Slight hijack, but a closely linked issue for the Pi.
The way I understand the issue of V4L2 / MC on Pi is a more
fundamental mismatch in architecture. Please correct me if I'm wrong
here.

The Pi CSI2 receiver peripheral always writes the incoming data to
SDRAM, and the ISP is then a memory to memory device.

V4L2 subdevices are not dma controllers and therefore have no buffers
allocated to them. So to support the full complexity of the pipeline
in V4L2 requires that something somewhere would have to be dequeuing
the buffers from the CSI receiver V4L2 device and queuing them to the
input of a (theoretical) ISP M2M V4L2 device, and returning them once
processed. The application only cares about the output of the ISP M2M
device.

So I guess my question is whether there is a sane mechanism to remove
that buffer allocation and handling from the app? Without it we are
pretty much forced to hide bigger blobs of functionality to even
vaguely fit in with V4L2.

I'm at the point where it shouldn't be a huge amount of work to create
at least a basic ISP V4L2 M2M device, but I'm not planning on doing it
if it pushes the above buffer handling onto the app because it simply
won't get used beyond demo apps. The likes of Cheese, Scratch, etc,
just won't do it.


To avoid ambiguity, the Pi has a hardware ISP block. There are other
SoCs that use either GPU code or a DSP to implement their ISP.

  Dave

Re: Compressed formats - framed or unframed?

[Dave Stevenson]

On 26 April 2018 at 17:25, Dave Stevenson
<dave.steven...@raspberrypi.org> wrote:
> Hi All.
>
> I'm trying to get a V4L2 M2M driver sorted for the Raspberry Pi to
> allow access to the video codecs. Much of it is working fine.
>
> One thing that isn't clear relates to video decode. Do the compressed
> formats (eg V4L2_PIX_FMT_H264) have to be framed into one frame per
> V4L2 buffer, or is providing unframed chunks of an elementary stream
> permitted. The docs only say "H264 video elementary stream with start
> codes.". Admittedly timestamps are nearly meaningless if you feed in
> unframed data, but could potentially be interpolated.
>
> What does other hardware support?
>
> I could handle it either way, but there are some performance tweaks I
> can do if I know the data must be framed.

Does anyone have any view on this?

Thanks
  Dave

Compressed formats - framed or unframed?

[Dave Stevenson]

Hi All.

I'm trying to get a V4L2 M2M driver sorted for the Raspberry Pi to
allow access to the video codecs. Much of it is working fine.

One thing that isn't clear relates to video decode. Do the compressed
formats (eg V4L2_PIX_FMT_H264) have to be framed into one frame per
V4L2 buffer, or is providing unframed chunks of an elementary stream
permitted. The docs only say "H264 video elementary stream with start
codes.". Admittedly timestamps are nearly meaningless if you feed in
unframed data, but could potentially be interpolated.

What does other hardware support?

I could handle it either way, but there are some performance tweaks I
can do if I know the data must be framed.

Thanks in advance.
  Dave

Re: [PATCH v3 2/4] [media] dt-bindings: Document BCM283x CSI2/CCP2 receiver

[Dave Stevenson]

On 21 November 2017 at 20:54, Eric Anholt <e...@anholt.net> wrote:
> Rob Herring <r...@kernel.org> writes:
>
>> On Tue, Nov 21, 2017 at 1:26 PM, Eric Anholt <e...@anholt.net> wrote:
>>> Dave Stevenson <dave.steven...@raspberrypi.org> writes:
>>>
>>>> Hi Rob
>>>>
>>>> On 27 September 2017 at 22:51, Rob Herring <r...@kernel.org> wrote:
>>>>> On Fri, Sep 22, 2017 at 05:07:22PM +0100, Dave Stevenson wrote:
>>>>>> Hi Stefan
>>>>>>
>>>>>> On 22 September 2017 at 07:45, Stefan Wahren <stefan.wah...@i2se.com> 
>>>>>> wrote:
>>>>>> > Hi Dave,
>>>>>> >
>>>>>> >> Dave Stevenson <dave.steven...@raspberrypi.org> hat am 20. September 
>>>>>> >> 2017 um 18:07 geschrieben:
>>>>>> >>
>>>>>> >>
>>>>>> >> Document the DT bindings for the CSI2/CCP2 receiver peripheral
>>>>>> >> (known as Unicam) on BCM283x SoCs.
>>>>>> >>
>>>>>> >> Signed-off-by: Dave Stevenson <dave.steven...@raspberrypi.org>
>>>>>> >> ---
>>>>>> >>
>>>>>> >> Changes since v2
>>>>>> >> - Removed all references to Linux drivers.
>>>>>> >> - Reworded section about disabling the firmware driver.
>>>>>> >> - Renamed clock from "lp_clock" to "lp" in description and example.
>>>>>> >> - Referred to video-interfaces.txt and stated requirements on 
>>>>>> >> remote-endpoint
>>>>>> >>   and data-lanes.
>>>>>> >> - Corrected typo in example from csi to csi1.
>>>>>> >> - Removed unnecessary #address-cells and #size-cells in example.
>>>>>> >> - Removed setting of status from the example.
>>>>>> >>
>>>>>> >>  .../devicetree/bindings/media/bcm2835-unicam.txt   | 85 
>>>>>> >> ++
>>>>>> >>  1 file changed, 85 insertions(+)
>>>>>> >>  create mode 100644 
>>>>>> >> Documentation/devicetree/bindings/media/bcm2835-unicam.txt
>>>>>> >>
>>>>>> >> diff --git 
>>>>>> >> a/Documentation/devicetree/bindings/media/bcm2835-unicam.txt 
>>>>>> >> b/Documentation/devicetree/bindings/media/bcm2835-unicam.txt
>>>>>> >> new file mode 100644
>>>>>> >> index 000..7714fb3
>>>>>> >> --- /dev/null
>>>>>> >> +++ b/Documentation/devicetree/bindings/media/bcm2835-unicam.txt
>>>>>> >> @@ -0,0 +1,85 @@
>>>>>> >> +Broadcom BCM283x Camera Interface (Unicam)
>>>>>> >> +--
>>>>>> >> +
>>>>>> >> +The Unicam block on BCM283x SoCs is the receiver for either
>>>>>> >> +CSI-2 or CCP2 data from image sensors or similar devices.
>>>>>> >> +
>>>>>> >> +The main platform using this SoC is the Raspberry Pi family of 
>>>>>> >> boards.
>>>>>> >> +On the Pi the VideoCore firmware can also control this hardware 
>>>>>> >> block,
>>>>>> >> +and driving it from two different processors will cause issues.
>>>>>> >> +To avoid this, the firmware checks the device tree configuration
>>>>>> >> +during boot. If it finds device tree nodes called csi0 or csi1 then
>>>>>> >> +it will stop the firmware accessing the block, and it can then
>>>>>> >> +safely be used via the device tree binding.
>>>>>> >> +
>>>>>> >> +Required properties:
>>>>>> >> +===
>>>>>> >> +- compatible : must be "brcm,bcm2835-unicam".
>>>>>> >> +- reg: physical base address and length of the 
>>>>>> >> register sets for the
>>>>>> >> +   device.
>>>>>> >> +- interrupts : should contain the IRQ line for this Unicam instance.
>>>>>> >> +- clocks : list of clock specifiers, corres

Re: [PATCH v3 1/4] [media] v4l2-common: Add helper function for fourcc to string

[Dave Stevenson]

Hi Sakari.

On 13 October 2017 at 22:09, Sakari Ailus <sakari.ai...@iki.fi> wrote:
> Hi Dave,
>
> On Wed, Sep 20, 2017 at 05:07:54PM +0100, Dave Stevenson wrote:
>> New helper function char *v4l2_fourcc2s(u32 fourcc, char *buf)
>> that converts a fourcc into a nice printable version.
>>
>> Signed-off-by: Dave Stevenson <dave.steven...@raspberrypi.org>
>> ---
>>
>> No changes from v2 to v3
>>
>>  drivers/media/v4l2-core/v4l2-common.c | 18 ++
>>  include/media/v4l2-common.h   |  3 +++
>>  2 files changed, 21 insertions(+)
>>
>> diff --git a/drivers/media/v4l2-core/v4l2-common.c 
>> b/drivers/media/v4l2-core/v4l2-common.c
>> index a5ea1f5..0219895 100644
>> --- a/drivers/media/v4l2-core/v4l2-common.c
>> +++ b/drivers/media/v4l2-core/v4l2-common.c
>> @@ -405,3 +405,21 @@ void v4l2_get_timestamp(struct timeval *tv)
>>   tv->tv_usec = ts.tv_nsec / NSEC_PER_USEC;
>>  }
>>  EXPORT_SYMBOL_GPL(v4l2_get_timestamp);
>> +
>> +char *v4l2_fourcc2s(u32 fourcc, char *buf)
>> +{
>> + buf[0] = fourcc & 0x7f;
>> + buf[1] = (fourcc >> 8) & 0x7f;
>> + buf[2] = (fourcc >> 16) & 0x7f;
>> + buf[3] = (fourcc >> 24) & 0x7f;
>> + if (fourcc & (1 << 31)) {
>> + buf[4] = '-';
>> + buf[5] = 'B';
>> + buf[6] = 'E';
>> + buf[7] = '\0';
>> + } else {
>> + buf[4] = '\0';
>> + }
>> + return buf;
>> +}
>> +EXPORT_SYMBOL_GPL(v4l2_fourcc2s);
>> diff --git a/include/media/v4l2-common.h b/include/media/v4l2-common.h
>> index aac8b7b..5b0fff9 100644
>> --- a/include/media/v4l2-common.h
>> +++ b/include/media/v4l2-common.h
>> @@ -264,4 +264,7 @@ const struct v4l2_frmsize_discrete 
>> *v4l2_find_nearest_format(
>>
>>  void v4l2_get_timestamp(struct timeval *tv);
>>
>> +#define V4L2_FOURCC_MAX_SIZE 8
>> +char *v4l2_fourcc2s(u32 fourcc, char *buf);
>> +
>>  #endif /* V4L2_COMMON_H_ */
>
> I like the idea but the use of a character pointer and assuming a length
> looks a bit scary.
>
> As this seems to be used uniquely for printing stuff, a couple of macros
> could be used instead. Something like
>
> #define V4L2_FOURCC_CONV "%c%c%c%c%s"
> #define V4L2_FOURCC_TO_CONV(fourcc) \
> fourcc & 0x7f, (fourcc >> 8) & 0x7f, (fourcc >> 16) & 0x7f, \
> (fourcc >> 24) & 0x7f, fourcc & BIT(31) ? "-BE" : ""
>
> You could use it with printk-style functions, e.g.
>
> printk("blah fourcc " V4L2_FOURCC_CONV " is a nice format",
>V4L2_FOURCC_TO_CONV(fourcc));
>
> I guess it'd be better to add more parentheses around "fourcc" but it'd be
> less clear here that way.

I was following Hans' suggestion made in
https://www.spinics.net/lists/linux-media/msg117046.html

Hans: Do you agree with Sakari here to make it to a macro?

  Dave

Re: [PATCH v3 2/4] [media] dt-bindings: Document BCM283x CSI2/CCP2 receiver

[Dave Stevenson]

Hi Rob

On 27 September 2017 at 22:51, Rob Herring <r...@kernel.org> wrote:
> On Fri, Sep 22, 2017 at 05:07:22PM +0100, Dave Stevenson wrote:
>> Hi Stefan
>>
>> On 22 September 2017 at 07:45, Stefan Wahren <stefan.wah...@i2se.com> wrote:
>> > Hi Dave,
>> >
>> >> Dave Stevenson <dave.steven...@raspberrypi.org> hat am 20. September 2017 
>> >> um 18:07 geschrieben:
>> >>
>> >>
>> >> Document the DT bindings for the CSI2/CCP2 receiver peripheral
>> >> (known as Unicam) on BCM283x SoCs.
>> >>
>> >> Signed-off-by: Dave Stevenson <dave.steven...@raspberrypi.org>
>> >> ---
>> >>
>> >> Changes since v2
>> >> - Removed all references to Linux drivers.
>> >> - Reworded section about disabling the firmware driver.
>> >> - Renamed clock from "lp_clock" to "lp" in description and example.
>> >> - Referred to video-interfaces.txt and stated requirements on 
>> >> remote-endpoint
>> >>   and data-lanes.
>> >> - Corrected typo in example from csi to csi1.
>> >> - Removed unnecessary #address-cells and #size-cells in example.
>> >> - Removed setting of status from the example.
>> >>
>> >>  .../devicetree/bindings/media/bcm2835-unicam.txt   | 85 
>> >> ++
>> >>  1 file changed, 85 insertions(+)
>> >>  create mode 100644 
>> >> Documentation/devicetree/bindings/media/bcm2835-unicam.txt
>> >>
>> >> diff --git a/Documentation/devicetree/bindings/media/bcm2835-unicam.txt 
>> >> b/Documentation/devicetree/bindings/media/bcm2835-unicam.txt
>> >> new file mode 100644
>> >> index 000..7714fb3
>> >> --- /dev/null
>> >> +++ b/Documentation/devicetree/bindings/media/bcm2835-unicam.txt
>> >> @@ -0,0 +1,85 @@
>> >> +Broadcom BCM283x Camera Interface (Unicam)
>> >> +--
>> >> +
>> >> +The Unicam block on BCM283x SoCs is the receiver for either
>> >> +CSI-2 or CCP2 data from image sensors or similar devices.
>> >> +
>> >> +The main platform using this SoC is the Raspberry Pi family of boards.
>> >> +On the Pi the VideoCore firmware can also control this hardware block,
>> >> +and driving it from two different processors will cause issues.
>> >> +To avoid this, the firmware checks the device tree configuration
>> >> +during boot. If it finds device tree nodes called csi0 or csi1 then
>> >> +it will stop the firmware accessing the block, and it can then
>> >> +safely be used via the device tree binding.
>> >> +
>> >> +Required properties:
>> >> +===
>> >> +- compatible : must be "brcm,bcm2835-unicam".
>> >> +- reg: physical base address and length of the register 
>> >> sets for the
>> >> +   device.
>> >> +- interrupts : should contain the IRQ line for this Unicam instance.
>> >> +- clocks : list of clock specifiers, corresponding to entries in
>> >> +   clock-names property.
>> >> +- clock-names: must contain an "lp" entry, matching entries in 
>> >> the
>> >> +   clocks property.
>> >> +
>> >> +Unicam supports a single port node. It should contain one 'port' child 
>> >> node
>> >> +with child 'endpoint' node. Please refer to the bindings defined in
>> >> +Documentation/devicetree/bindings/media/video-interfaces.txt.
>> >> +
>> >> +Within the endpoint node the "remote-endpoint" and "data-lanes" 
>> >> properties
>> >> +are mandatory.
>> >> +Data lane reordering is not supported so the data lanes must be in order,
>> >> +starting at 1. The number of data lanes should represent the number of
>> >> +usable lanes for the hardware block. That may be limited by either the 
>> >> SoC or
>> >> +how the platform presents the interface, and the lower value must be 
>> >> used.
>> >> +
>> >> +Lane reordering is not supported on the clock lane either, so the 
>> >> optional
>> >> +property "clock-lane" will implicitly be <0>.
>> >> +Similarly lane inversion is not supported, therefore "lane-polarities" 
>> >>

Re: [PATCH v2 1/2] [media] tc358743: fix connected/active CSI-2 lane reporting

[Dave Stevenson]

On 21 September 2017 at 16:30, Philipp Zabel <p.za...@pengutronix.de> wrote:
> g_mbus_config was supposed to indicate all supported lane numbers, not
> only the number of those currently in active use. Since the TC358743
> can dynamically reduce the number of active lanes if the required
> bandwidth allows for it, report all lane numbers up to the connected
> number of lanes as supported in pdata mode.
> In device tree mode, do not report lane count and clock mode at all, as
> the receiver driver can determine these from the device tree.
>
> To allow communicating the number of currently active lanes, add a new
> bitfield to the v4l2_mbus_config flags. This is a temporary fix, to be
> used only until a better solution is found.
>
> Signed-off-by: Philipp Zabel <p.za...@pengutronix.de>

Tested-by: Dave Stevenson <dave.steven...@raspberrypi.org>

> ---
> Changes since v1:
>  - Check csi_lanes_in_use <= num_data_lanes before writing to cfg.
>  - Increase size of lane mask to 4 bits and always explicitly report
>number of lanes in use.
>  - Clear clock and connected lane flags in DT mode.
> ---
>  drivers/media/i2c/tc358743.c  | 30 --
>  include/media/v4l2-mediabus.h |  8 
>  2 files changed, 24 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/media/i2c/tc358743.c b/drivers/media/i2c/tc358743.c
> index e6f5c363ccab5..a35043cefe128 100644
> --- a/drivers/media/i2c/tc358743.c
> +++ b/drivers/media/i2c/tc358743.c
> @@ -1458,28 +1458,29 @@ static int tc358743_g_mbus_config(struct v4l2_subdev 
> *sd,
>  struct v4l2_mbus_config *cfg)
>  {
> struct tc358743_state *state = to_state(sd);
> +   const u32 mask = V4L2_MBUS_CSI2_LANE_MASK;
> +
> +   if (state->csi_lanes_in_use > state->bus.num_data_lanes)
> +   return -EINVAL;
>
> cfg->type = V4L2_MBUS_CSI2;
> +   cfg->flags = (state->csi_lanes_in_use << __ffs(mask)) & mask;
>
> -   /* Support for non-continuous CSI-2 clock is missing in the driver */
> -   cfg->flags = V4L2_MBUS_CSI2_CONTINUOUS_CLOCK;
> +   /* In DT mode, only report the number of active lanes */
> +   if (sd->dev->of_node)
> +   return 0;
>
> -   switch (state->csi_lanes_in_use) {
> -   case 1:
> +   /* Support for non-continuous CSI-2 clock is missing in pdata mode */
> +   cfg->flags |= V4L2_MBUS_CSI2_CONTINUOUS_CLOCK;
> +
> +   if (state->bus.num_data_lanes > 0)
> cfg->flags |= V4L2_MBUS_CSI2_1_LANE;
> -   break;
> -   case 2:
> +   if (state->bus.num_data_lanes > 1)
> cfg->flags |= V4L2_MBUS_CSI2_2_LANE;
> -   break;
> -   case 3:
> +   if (state->bus.num_data_lanes > 2)
> cfg->flags |= V4L2_MBUS_CSI2_3_LANE;
> -   break;
> -   case 4:
> +   if (state->bus.num_data_lanes > 3)
> cfg->flags |= V4L2_MBUS_CSI2_4_LANE;
> -   break;
> -   default:
> -   return -EINVAL;
> -   }
>
> return 0;
>  }
> @@ -1885,6 +1886,7 @@ static int tc358743_probe(struct i2c_client *client,
> if (pdata) {
> state->pdata = *pdata;
> state->bus.flags = V4L2_MBUS_CSI2_CONTINUOUS_CLOCK;
> +   state->bus.num_data_lanes = 4;
> } else {
> err = tc358743_probe_of(state);
> if (err == -ENODEV)
> diff --git a/include/media/v4l2-mediabus.h b/include/media/v4l2-mediabus.h
> index 93f8afcb7a220..fc106c902bf47 100644
> --- a/include/media/v4l2-mediabus.h
> +++ b/include/media/v4l2-mediabus.h
> @@ -63,6 +63,14 @@
>  V4L2_MBUS_CSI2_3_LANE | 
> V4L2_MBUS_CSI2_4_LANE)
>  #define V4L2_MBUS_CSI2_CHANNELS(V4L2_MBUS_CSI2_CHANNEL_0 | 
> V4L2_MBUS_CSI2_CHANNEL_1 | \
>  V4L2_MBUS_CSI2_CHANNEL_2 | 
> V4L2_MBUS_CSI2_CHANNEL_3)
> +/*
> + * Number of lanes in use, 0 == use all available lanes (default)
> + *
> + * This is a temporary fix for devices that need to reduce the number of 
> active
> + * lanes for certain modes, until g_mbus_config() can be replaced with a 
> better
> + * solution.
> + */
> +#define V4L2_MBUS_CSI2_LANE_MASK(0xf << 10)
>
>  /**
>   * enum v4l2_mbus_type - media bus type
> --
> 2.11.0
>

Re: [PATCH v2 0/4] BCM283x Camera Receiver driver

[Dave Stevenson]

On 22 September 2017 at 18:17, Hans Verkuil <hverk...@xs4all.nl> wrote:
> On 22/09/17 18:31, Dave Stevenson wrote:
>> On 22 September 2017 at 12:00, Hans Verkuil <hverk...@xs4all.nl> wrote:
>>> On 13/09/17 17:49, Dave Stevenson wrote:
>>>> OV5647
>>>>
>>>> v4l2-compliance SHA   : f6ecbc90656815d91dc6ba90aac0ad8193a14b38
>>>>
>>>> Driver Info:
>>>> Driver name   : unicam
>>>> Card type : unicam
>>>> Bus info  : platform:unicam 3f801000.csi1
>>>> Driver version: 4.13.0
>>>> Capabilities  : 0x8521
>>>> Video Capture
>>>> Read/Write
>>>> Streaming
>>>> Extended Pix Format
>>>> Device Capabilities
>>>> Device Caps   : 0x0521
>>>> Video Capture
>>>> Read/Write
>>>> Streaming
>>>> Extended Pix Format
>>>>
>>>> Compliance test for device /dev/video0 (not using libv4l2):
>>>>
>>>> Required ioctls:
>>>> test VIDIOC_QUERYCAP: OK
>>>>
>>>> Allow for multiple opens:
>>>> test second video open: OK
>>>> test VIDIOC_QUERYCAP: OK
>>>> test VIDIOC_G/S_PRIORITY: OK
>>>> test for unlimited opens: OK
>>>>
>>>> Debug ioctls:
>>>> test VIDIOC_DBG_G/S_REGISTER: OK (Not Supported)
>>>> test VIDIOC_LOG_STATUS: OK
>>>>
>>>> Input ioctls:
>>>> test VIDIOC_G/S_TUNER/ENUM_FREQ_BANDS: OK (Not Supported)
>>>> test VIDIOC_G/S_FREQUENCY: OK (Not Supported)
>>>> test VIDIOC_S_HW_FREQ_SEEK: OK (Not Supported)
>>>> test VIDIOC_ENUMAUDIO: OK (Not Supported)
>>>> test VIDIOC_G/S/ENUMINPUT: OK
>>>> test VIDIOC_G/S_AUDIO: OK (Not Supported)
>>>> Inputs: 1 Audio Inputs: 0 Tuners: 0
>>>>
>>>> Output ioctls:
>>>> test VIDIOC_G/S_MODULATOR: OK (Not Supported)
>>>> test VIDIOC_G/S_FREQUENCY: OK (Not Supported)
>>>> test VIDIOC_ENUMAUDOUT: OK (Not Supported)
>>>> test VIDIOC_G/S/ENUMOUTPUT: OK (Not Supported)
>>>> test VIDIOC_G/S_AUDOUT: OK (Not Supported)
>>>> Outputs: 0 Audio Outputs: 0 Modulators: 0
>>>>
>>>> Input/Output configuration ioctls:
>>>> test VIDIOC_ENUM/G/S/QUERY_STD: OK (Not Supported)
>>>> test VIDIOC_ENUM/G/S/QUERY_DV_TIMINGS: OK (Not Supported)
>>>> test VIDIOC_DV_TIMINGS_CAP: OK (Not Supported)
>>>> test VIDIOC_G/S_EDID: OK (Not Supported)
>>>>
>>>> Test input 0:
>>>>
>>>> Control ioctls:
>>>> test VIDIOC_QUERY_EXT_CTRL/QUERYMENU: OK
>>>> test VIDIOC_QUERYCTRL: OK
>>>> test VIDIOC_G/S_CTRL: OK
>>>> fail: v4l2-test-controls.cpp(587): g_ext_ctrls does not
>>>> support count == 0
>>>
>>> Huh. The issue here is that there are no controls at all, but the
>>> control API is present. The class_check() function in v4l2-ctrls.c expects
>>> that there are controls and if not it returns -EINVAL, causing this test
>>> to fail.
>>>
>>> Try to apply this patch:
>>>
>>> Signed-off-by: Hans Verkuil <hans.verk...@cisco.com>
>>> ---
>>> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c 
>>> b/drivers/media/v4l2-core/v4l2-ctrls.c
>>> index dd1db678718c..4e53a8654690 100644
>>> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
>>> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
>>> @@ -2818,7 +2818,7 @@ static int prepare_ext_ctrls(struct v4l2_ctrl_handler 
>>> *hdl,
>>>  static int class_check(struct v4l2_ctrl_handler *hdl, u32 which)
>>>  {
>>> if (which == 0 || which == V4L2_CTRL_WHICH_DEF_VAL)
>>> -   return list_empty(>ctrl_refs) ? -EINVAL : 0;
>>> +   return 0;
>>> return find_ref_lock(hdl, which | 1) ? 0 : -EINVAL;
>>>  }
>>>
>>>
>>> and see if it will pass the compliance test. There may be other issues.
>>> I think that the compliance test should handle the case where there are no
>>> controls, so this is a good test.
>>
>> Fails.
>> fail: v4l2-test-controls.cpp(589): g_ext_ctrls worked even
>> when no controls are present
>> test VIDIOC_G/S/TRY

Re: [PATCH v2 0/4] BCM283x Camera Receiver driver

[Dave Stevenson]

On 22 September 2017 at 12:00, Hans Verkuil <hverk...@xs4all.nl> wrote:
> On 13/09/17 17:49, Dave Stevenson wrote:
>> OV5647
>>
>> v4l2-compliance SHA   : f6ecbc90656815d91dc6ba90aac0ad8193a14b38
>>
>> Driver Info:
>> Driver name   : unicam
>> Card type : unicam
>> Bus info  : platform:unicam 3f801000.csi1
>> Driver version: 4.13.0
>> Capabilities  : 0x8521
>> Video Capture
>> Read/Write
>> Streaming
>> Extended Pix Format
>> Device Capabilities
>> Device Caps   : 0x0521
>> Video Capture
>> Read/Write
>> Streaming
>> Extended Pix Format
>>
>> Compliance test for device /dev/video0 (not using libv4l2):
>>
>> Required ioctls:
>> test VIDIOC_QUERYCAP: OK
>>
>> Allow for multiple opens:
>> test second video open: OK
>> test VIDIOC_QUERYCAP: OK
>> test VIDIOC_G/S_PRIORITY: OK
>> test for unlimited opens: OK
>>
>> Debug ioctls:
>> test VIDIOC_DBG_G/S_REGISTER: OK (Not Supported)
>> test VIDIOC_LOG_STATUS: OK
>>
>> Input ioctls:
>> test VIDIOC_G/S_TUNER/ENUM_FREQ_BANDS: OK (Not Supported)
>> test VIDIOC_G/S_FREQUENCY: OK (Not Supported)
>> test VIDIOC_S_HW_FREQ_SEEK: OK (Not Supported)
>> test VIDIOC_ENUMAUDIO: OK (Not Supported)
>> test VIDIOC_G/S/ENUMINPUT: OK
>> test VIDIOC_G/S_AUDIO: OK (Not Supported)
>> Inputs: 1 Audio Inputs: 0 Tuners: 0
>>
>> Output ioctls:
>> test VIDIOC_G/S_MODULATOR: OK (Not Supported)
>> test VIDIOC_G/S_FREQUENCY: OK (Not Supported)
>> test VIDIOC_ENUMAUDOUT: OK (Not Supported)
>> test VIDIOC_G/S/ENUMOUTPUT: OK (Not Supported)
>> test VIDIOC_G/S_AUDOUT: OK (Not Supported)
>> Outputs: 0 Audio Outputs: 0 Modulators: 0
>>
>> Input/Output configuration ioctls:
>> test VIDIOC_ENUM/G/S/QUERY_STD: OK (Not Supported)
>> test VIDIOC_ENUM/G/S/QUERY_DV_TIMINGS: OK (Not Supported)
>> test VIDIOC_DV_TIMINGS_CAP: OK (Not Supported)
>> test VIDIOC_G/S_EDID: OK (Not Supported)
>>
>> Test input 0:
>>
>> Control ioctls:
>> test VIDIOC_QUERY_EXT_CTRL/QUERYMENU: OK
>> test VIDIOC_QUERYCTRL: OK
>> test VIDIOC_G/S_CTRL: OK
>> fail: v4l2-test-controls.cpp(587): g_ext_ctrls does not
>> support count == 0
>
> Huh. The issue here is that there are no controls at all, but the
> control API is present. The class_check() function in v4l2-ctrls.c expects
> that there are controls and if not it returns -EINVAL, causing this test
> to fail.
>
> Try to apply this patch:
>
> Signed-off-by: Hans Verkuil <hans.verk...@cisco.com>
> ---
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c 
> b/drivers/media/v4l2-core/v4l2-ctrls.c
> index dd1db678718c..4e53a8654690 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> @@ -2818,7 +2818,7 @@ static int prepare_ext_ctrls(struct v4l2_ctrl_handler 
> *hdl,
>  static int class_check(struct v4l2_ctrl_handler *hdl, u32 which)
>  {
> if (which == 0 || which == V4L2_CTRL_WHICH_DEF_VAL)
> -   return list_empty(>ctrl_refs) ? -EINVAL : 0;
> +   return 0;
> return find_ref_lock(hdl, which | 1) ? 0 : -EINVAL;
>  }
>
>
> and see if it will pass the compliance test. There may be other issues.
> I think that the compliance test should handle the case where there are no
> controls, so this is a good test.

Fails.
fail: v4l2-test-controls.cpp(589): g_ext_ctrls worked even
when no controls are present
test VIDIOC_G/S/TRY_EXT_CTRLS: FAIL

> That said, another solution is that the driver detects that there are no
> controls in unicam_probe_complete() and sets unicam->v4l2_dev.ctrl_handler
> to NULL.
>
> I think you should do this in v4. Having control ioctls but no actual controls
> is not wrong as such, but it is a bit misleading towards the application.

OK, will do.

  Dave

Re: [PATCH v3 2/4] [media] dt-bindings: Document BCM283x CSI2/CCP2 receiver

[Dave Stevenson]

Hi Stefan

On 22 September 2017 at 07:45, Stefan Wahren <stefan.wah...@i2se.com> wrote:
> Hi Dave,
>
>> Dave Stevenson <dave.steven...@raspberrypi.org> hat am 20. September 2017 um 
>> 18:07 geschrieben:
>>
>>
>> Document the DT bindings for the CSI2/CCP2 receiver peripheral
>> (known as Unicam) on BCM283x SoCs.
>>
>> Signed-off-by: Dave Stevenson <dave.steven...@raspberrypi.org>
>> ---
>>
>> Changes since v2
>> - Removed all references to Linux drivers.
>> - Reworded section about disabling the firmware driver.
>> - Renamed clock from "lp_clock" to "lp" in description and example.
>> - Referred to video-interfaces.txt and stated requirements on remote-endpoint
>>   and data-lanes.
>> - Corrected typo in example from csi to csi1.
>> - Removed unnecessary #address-cells and #size-cells in example.
>> - Removed setting of status from the example.
>>
>>  .../devicetree/bindings/media/bcm2835-unicam.txt   | 85 
>> ++
>>  1 file changed, 85 insertions(+)
>>  create mode 100644 
>> Documentation/devicetree/bindings/media/bcm2835-unicam.txt
>>
>> diff --git a/Documentation/devicetree/bindings/media/bcm2835-unicam.txt 
>> b/Documentation/devicetree/bindings/media/bcm2835-unicam.txt
>> new file mode 100644
>> index 000..7714fb3
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/media/bcm2835-unicam.txt
>> @@ -0,0 +1,85 @@
>> +Broadcom BCM283x Camera Interface (Unicam)
>> +--
>> +
>> +The Unicam block on BCM283x SoCs is the receiver for either
>> +CSI-2 or CCP2 data from image sensors or similar devices.
>> +
>> +The main platform using this SoC is the Raspberry Pi family of boards.
>> +On the Pi the VideoCore firmware can also control this hardware block,
>> +and driving it from two different processors will cause issues.
>> +To avoid this, the firmware checks the device tree configuration
>> +during boot. If it finds device tree nodes called csi0 or csi1 then
>> +it will stop the firmware accessing the block, and it can then
>> +safely be used via the device tree binding.
>> +
>> +Required properties:
>> +===
>> +- compatible : must be "brcm,bcm2835-unicam".
>> +- reg: physical base address and length of the register 
>> sets for the
>> +   device.
>> +- interrupts : should contain the IRQ line for this Unicam instance.
>> +- clocks : list of clock specifiers, corresponding to entries in
>> +   clock-names property.
>> +- clock-names: must contain an "lp" entry, matching entries in the
>> +   clocks property.
>> +
>> +Unicam supports a single port node. It should contain one 'port' child node
>> +with child 'endpoint' node. Please refer to the bindings defined in
>> +Documentation/devicetree/bindings/media/video-interfaces.txt.
>> +
>> +Within the endpoint node the "remote-endpoint" and "data-lanes" properties
>> +are mandatory.
>> +Data lane reordering is not supported so the data lanes must be in order,
>> +starting at 1. The number of data lanes should represent the number of
>> +usable lanes for the hardware block. That may be limited by either the SoC 
>> or
>> +how the platform presents the interface, and the lower value must be used.
>> +
>> +Lane reordering is not supported on the clock lane either, so the optional
>> +property "clock-lane" will implicitly be <0>.
>> +Similarly lane inversion is not supported, therefore "lane-polarities" will
>> +implicitly be <0 0 0 0 0>.
>> +Neither of these values will be checked.
>> +
>> +Example:
>> + csi1: csi1@7e801000 {
>> + compatible = "brcm,bcm2835-unicam";
>> + reg = <0x7e801000 0x800>,
>> +   <0x7e802004 0x4>;
>
> sorry, i didn't noticed this before. I'm afraid this is using a small range 
> of the CMI. Are there possible other users of this range? Does it make sense 
> to handle this by a separate clock driver?

CMI (Clock Manager Image) consists of a total of 4 registers.
0x7e802000 is CMI_CAM0, with only bits 0-5 used for gating and
inversion of the clock and data lanes (2 data lanes available on
CAM0).
0x7e802004 is CMI_CAM1, with only bits 0-9 used for gating and
inversion of the clock and data lanes (4 data lanes available on
CAM1).
0x7e802008 is CMI_CAMTEST which I have no documentation or drivers for.
0x7e802010 is CMI_USBCTL. Only bit 6 is documented and is a reset. The
default value is the required value. Nothing touches it that I can
find.

The range listed only covers the one register associated with that
Unicam instance, so no other users. The other two aren't touched.
Do 16 active register bits solely for camera clock gating really
warrant a full clock driver?

  Dave

Re: [PATCH] tc358743: fix connected/active CSI-2 lane reporting

[Dave Stevenson]

Hi Philipp

On 21 September 2017 at 11:24, Philipp Zabel  wrote:
> g_mbus_config was supposed to indicate all supported lane numbers, not
> only the number of those currently in active use. Since the tc358743
> can dynamically reduce the number of active lanes if the required
> bandwidth allows for it, report all lane numbers up to the connected
> number of lanes as supported.
> To allow communicating the number of currently active lanes, add a new
> bitfield to the v4l2_mbus_config flags. This is a temporary fix, until
> a better solution is found.
>
> Signed-off-by: Philipp Zabel 
> ---
>  drivers/media/i2c/tc358743.c  | 22 --
>  include/media/v4l2-mediabus.h |  8 
>  2 files changed, 20 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/media/i2c/tc358743.c b/drivers/media/i2c/tc358743.c
> index e6f5c363ccab5..e2a9e6a18a49d 100644
> --- a/drivers/media/i2c/tc358743.c
> +++ b/drivers/media/i2c/tc358743.c
> @@ -1464,21 +1464,22 @@ static int tc358743_g_mbus_config(struct v4l2_subdev 
> *sd,
> /* Support for non-continuous CSI-2 clock is missing in the driver */
> cfg->flags = V4L2_MBUS_CSI2_CONTINUOUS_CLOCK;
>
> -   switch (state->csi_lanes_in_use) {
> -   case 1:
> +   if (state->bus.num_data_lanes > 0)
> cfg->flags |= V4L2_MBUS_CSI2_1_LANE;
> -   break;
> -   case 2:
> +   if (state->bus.num_data_lanes > 1)
> cfg->flags |= V4L2_MBUS_CSI2_2_LANE;
> -   break;
> -   case 3:
> +   if (state->bus.num_data_lanes > 2)
> cfg->flags |= V4L2_MBUS_CSI2_3_LANE;
> -   break;
> -   case 4:
> +   if (state->bus.num_data_lanes > 3)
> cfg->flags |= V4L2_MBUS_CSI2_4_LANE;
> -   break;
> -   default:
> +
> +   if (state->csi_lanes_in_use > 4)

One could suggest
if (state->csi_lanes_in_use > state->bus.num_data_lanes)
here. Needing to use more lanes than are configured is surely an
error, although that may be detectable at the other end. See below
too.

> return -EINVAL;
> +
> +   if (state->csi_lanes_in_use < state->bus.num_data_lanes) {
> +   const u32 mask = V4L2_MBUS_CSI2_LANE_MASK;
> +
> +   cfg->flags |= (state->csi_lanes_in_use << __ffs(mask)) & mask;
> }
>
> return 0;
> @@ -1885,6 +1886,7 @@ static int tc358743_probe(struct i2c_client *client,
> if (pdata) {
> state->pdata = *pdata;
> state->bus.flags = V4L2_MBUS_CSI2_CONTINUOUS_CLOCK;
> +   state->bus.num_data_lanes = 4;
> } else {
> err = tc358743_probe_of(state);
> if (err == -ENODEV)
> diff --git a/include/media/v4l2-mediabus.h b/include/media/v4l2-mediabus.h
> index 93f8afcb7a220..3467d97be5f5b 100644
> --- a/include/media/v4l2-mediabus.h
> +++ b/include/media/v4l2-mediabus.h
> @@ -63,6 +63,14 @@
>  V4L2_MBUS_CSI2_3_LANE | 
> V4L2_MBUS_CSI2_4_LANE)
>  #define V4L2_MBUS_CSI2_CHANNELS(V4L2_MBUS_CSI2_CHANNEL_0 | 
> V4L2_MBUS_CSI2_CHANNEL_1 | \
>  V4L2_MBUS_CSI2_CHANNEL_2 | 
> V4L2_MBUS_CSI2_CHANNEL_3)
> +/*
> + * Number of lanes in use, 0 == use all available lanes (default)
> + *
> + * This is a temporary fix for devices that need to reduce the number of 
> active
> + * lanes for certain modes, until g_mbus_config() can be replaced with a 
> better
> + * solution.
> + */
> +#define V4L2_MBUS_CSI2_LANE_MASK(3 << 10)

I know this was Hans' suggested define, but are we saying 4 lanes is
not a valid value? If it is then the mask needs to be at least (7 <<
10).

4 lanes is not necessarily "all available lanes".
- There are now CSI2 devices supporting up to 8 lanes (although
V4L2_FWNODE_CSI2_MAX_DATA_LANES limits you to 4 at the moment).
- Or you could have 2 lanes configured in DT and ask TC358743 for (eg)
1080P60 UYVY at 594Mbps (needs 4 lanes) which passes the current logic
in the TC358743 driver and would return 0, when it is actually going
to use 4 lanes. That could be classed as a driver bug though.

My view is that if a driver is going to report how many lanes to use
then it should always report it explicitly. The default 0 value should
only be used for devices that will never change it from the DT
settings. Perhaps others disagree

Otherwise the patch works for me.

  Dave.

>  /**
>   * enum v4l2_mbus_type - media bus type
> --
> 2.11.0
>

[PATCH v3 3/4] [media] bcm2835-unicam: Driver for CCP2/CSI2 camera interface

[Dave Stevenson]

Add driver for the Unicam camera receiver block on
BCM283x processors.

Signed-off-by: Dave Stevenson <dave.steven...@raspberrypi.org>
---

Changes from v2.
- Don't store the irq value as it isn't used outside the probe.
- Change PIX_FMT_ALL_ defines to avoid potential clashes with 4CCs.
- Clock now called "lp" and not "lp_clock".
- Minor description changes.

 drivers/media/platform/Kconfig   |1 +
 drivers/media/platform/Makefile  |1 +
 drivers/media/platform/bcm2835/Kconfig   |   14 +
 drivers/media/platform/bcm2835/Makefile  |3 +
 drivers/media/platform/bcm2835/bcm2835-unicam.c  | 2192 ++
 drivers/media/platform/bcm2835/vc4-regs-unicam.h |  264 +++
 6 files changed, 2475 insertions(+)
 create mode 100644 drivers/media/platform/bcm2835/Kconfig
 create mode 100644 drivers/media/platform/bcm2835/Makefile
 create mode 100644 drivers/media/platform/bcm2835/bcm2835-unicam.c
 create mode 100644 drivers/media/platform/bcm2835/vc4-regs-unicam.h

diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
index 7e7cc49..1e5f004 100644
--- a/drivers/media/platform/Kconfig
+++ b/drivers/media/platform/Kconfig
@@ -150,6 +150,7 @@ source "drivers/media/platform/am437x/Kconfig"
 source "drivers/media/platform/xilinx/Kconfig"
 source "drivers/media/platform/rcar-vin/Kconfig"
 source "drivers/media/platform/atmel/Kconfig"
+source "drivers/media/platform/bcm2835/Kconfig"
 
 config VIDEO_TI_CAL
tristate "TI CAL (Camera Adaptation Layer) driver"
diff --git a/drivers/media/platform/Makefile b/drivers/media/platform/Makefile
index c1ef946..045de3f 100644
--- a/drivers/media/platform/Makefile
+++ b/drivers/media/platform/Makefile
@@ -90,3 +90,4 @@ obj-$(CONFIG_VIDEO_QCOM_CAMSS)+= 
qcom/camss-8x16/
 obj-$(CONFIG_VIDEO_QCOM_VENUS) += qcom/venus/
 
 obj-y  += meson/
+obj-y  += bcm2835/
diff --git a/drivers/media/platform/bcm2835/Kconfig 
b/drivers/media/platform/bcm2835/Kconfig
new file mode 100644
index 000..6a74842
--- /dev/null
+++ b/drivers/media/platform/bcm2835/Kconfig
@@ -0,0 +1,14 @@
+# Broadcom VideoCore4 V4L2 camera support
+
+config VIDEO_BCM2835_UNICAM
+   tristate "Broadcom BCM2835 Unicam video capture driver"
+   depends on VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API
+   depends on ARCH_BCM2835 || COMPILE_TEST
+   select VIDEOBUF2_DMA_CONTIG
+   select V4L2_FWNODE
+   ---help---
+ Say Y here to enable V4L2 subdevice for CSI2 receiver.
+ This is a V4L2 subdevice that interfaces directly to the VC4 
peripheral.
+
+  To compile this driver as a module, choose M here. The module
+  will be called bcm2835-unicam.
diff --git a/drivers/media/platform/bcm2835/Makefile 
b/drivers/media/platform/bcm2835/Makefile
new file mode 100644
index 000..a98aba0
--- /dev/null
+++ b/drivers/media/platform/bcm2835/Makefile
@@ -0,0 +1,3 @@
+# Makefile for BCM2835 Unicam driver
+
+obj-$(CONFIG_VIDEO_BCM2835_UNICAM) += bcm2835-unicam.o
diff --git a/drivers/media/platform/bcm2835/bcm2835-unicam.c 
b/drivers/media/platform/bcm2835/bcm2835-unicam.c
new file mode 100644
index 000..93831fb
--- /dev/null
+++ b/drivers/media/platform/bcm2835/bcm2835-unicam.c
@@ -0,0 +1,2192 @@
+/*
+ * BCM2835 Unicam capture Driver
+ *
+ * Copyright (C) 2017 - Raspberry Pi (Trading) Ltd.
+ *
+ * Dave Stevenson <dave.steven...@raspberrypi.org>
+ *
+ * Based on TI am437x driver by Benoit Parrot and Lad, Prabhakar and
+ * TI CAL camera interface driver by Benoit Parrot.
+ *
+ *
+ * There are two camera drivers in the kernel for BCM283x - this one
+ * and bcm2835-camera (currently in staging).
+ *
+ * This driver directly controls the Unicam peripheral - there is no
+ * involvement with the VideoCore firmware. Unicam receives CSI-2 or
+ * CCP2 data and writes it into SDRAM. The only processing options are
+ * to repack Bayer data into an alternate format, and applying windowing
+ * (currently not implemented).
+ * It should be possible to connect it to any sensor with a
+ * suitable output interface and V4L2 subdevice driver.
+ *
+ * bcm2835-camera uses the VideoCore firmware to control the sensor,
+ * Unicam, ISP, and all tuner control loops. Fully processed frames are
+ * delivered to the driver by the firmware. It only has sensor drivers
+ * for Omnivision OV5647, and Sony IMX219 sensors.
+ *
+ * The two drivers are mutually exclusive for the same Unicam instance.
+ * The VideoCore firmware checks the device tree configuration during boot.
+ * If it finds device tree nodes called csi0 or csi1 it will block the
+ * firmware from accessing the peripheral, and bcm2835-camera will
+ * not be able to stream data.
+ *
+ *
+ * This program is free software; you may redistribute it and/or modify
+ * it under the term

[PATCH v3 4/4] MAINTAINERS: Add entry for BCM2835 camera driver

[Dave Stevenson]

Signed-off-by: Dave Stevenson <dave.steven...@raspberrypi.org>
---

No changes from v2 to v3

 MAINTAINERS | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index eb930eb..b47ddaa 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2713,6 +2713,13 @@ S:   Maintained
 N: bcm2835
 F: drivers/staging/vc04_services
 
+BROADCOM BCM2835 CAMERA DRIVER
+M:     Dave Stevenson <dave.steven...@raspberrypi.org>
+L: linux-media@vger.kernel.org
+S: Maintained
+F: drivers/media/platform/bcm2835/
+F: Documentation/devicetree/bindings/media/bcm2835-unicam.txt
+
 BROADCOM BCM47XX MIPS ARCHITECTURE
 M: Hauke Mehrtens <ha...@hauke-m.de>
 M: Rafał Miłecki <zaj...@gmail.com>
-- 
2.7.4

[PATCH v3 2/4] [media] dt-bindings: Document BCM283x CSI2/CCP2 receiver

[Dave Stevenson]

Document the DT bindings for the CSI2/CCP2 receiver peripheral
(known as Unicam) on BCM283x SoCs.

Signed-off-by: Dave Stevenson <dave.steven...@raspberrypi.org>
---

Changes since v2
- Removed all references to Linux drivers.
- Reworded section about disabling the firmware driver.
- Renamed clock from "lp_clock" to "lp" in description and example.
- Referred to video-interfaces.txt and stated requirements on remote-endpoint
  and data-lanes.
- Corrected typo in example from csi to csi1.
- Removed unnecessary #address-cells and #size-cells in example.
- Removed setting of status from the example.

 .../devicetree/bindings/media/bcm2835-unicam.txt   | 85 ++
 1 file changed, 85 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/bcm2835-unicam.txt

diff --git a/Documentation/devicetree/bindings/media/bcm2835-unicam.txt 
b/Documentation/devicetree/bindings/media/bcm2835-unicam.txt
new file mode 100644
index 000..7714fb3
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/bcm2835-unicam.txt
@@ -0,0 +1,85 @@
+Broadcom BCM283x Camera Interface (Unicam)
+--
+
+The Unicam block on BCM283x SoCs is the receiver for either
+CSI-2 or CCP2 data from image sensors or similar devices.
+
+The main platform using this SoC is the Raspberry Pi family of boards.
+On the Pi the VideoCore firmware can also control this hardware block,
+and driving it from two different processors will cause issues.
+To avoid this, the firmware checks the device tree configuration
+during boot. If it finds device tree nodes called csi0 or csi1 then
+it will stop the firmware accessing the block, and it can then
+safely be used via the device tree binding.
+
+Required properties:
+===
+- compatible   : must be "brcm,bcm2835-unicam".
+- reg  : physical base address and length of the register sets for the
+ device.
+- interrupts   : should contain the IRQ line for this Unicam instance.
+- clocks   : list of clock specifiers, corresponding to entries in
+ clock-names property.
+- clock-names  : must contain an "lp" entry, matching entries in the
+ clocks property.
+
+Unicam supports a single port node. It should contain one 'port' child node
+with child 'endpoint' node. Please refer to the bindings defined in
+Documentation/devicetree/bindings/media/video-interfaces.txt.
+
+Within the endpoint node the "remote-endpoint" and "data-lanes" properties
+are mandatory.
+Data lane reordering is not supported so the data lanes must be in order,
+starting at 1. The number of data lanes should represent the number of
+usable lanes for the hardware block. That may be limited by either the SoC or
+how the platform presents the interface, and the lower value must be used.
+
+Lane reordering is not supported on the clock lane either, so the optional
+property "clock-lane" will implicitly be <0>.
+Similarly lane inversion is not supported, therefore "lane-polarities" will
+implicitly be <0 0 0 0 0>.
+Neither of these values will be checked.
+
+Example:
+   csi1: csi1@7e801000 {
+   compatible = "brcm,bcm2835-unicam";
+   reg = <0x7e801000 0x800>,
+ <0x7e802004 0x4>;
+   interrupts = <2 7>;
+   clocks = < BCM2835_CLOCK_CAM1>;
+   clock-names = "lp";
+
+   port {
+   csi1_ep: endpoint {
+   remote-endpoint = <_0>;
+   data-lanes = <1 2>;
+   };
+   };
+   };
+
+   i2c0: i2c@7e205000 {
+   tc358743: csi-hdmi-bridge@0f {
+   compatible = "toshiba,tc358743";
+   reg = <0x0f>;
+
+   clocks = <_clk>;
+   clock-names = "refclk";
+
+   tc358743_clk: bridge-clk {
+   compatible = "fixed-clock";
+   #clock-cells = <0>;
+   clock-frequency = <2700>;
+   };
+
+   port {
+   tc358743_0: endpoint {
+   remote-endpoint = <_ep>;
+   clock-lanes = <0>;
+   data-lanes = <1 2>;
+   clock-noncontinuous;
+   link-frequencies =
+   /bits/ 64 <29700>;
+   };
+   };
+   };
+   };
-- 
2.7.4

[PATCH v3 0/4] BCM283x Camera Receiver driver

[Dave Stevenson]

Hi All.

v3 of this patch set.
This addresses the DT issues raised by Rob, and the things raised by
Eric on the driver. More complete change details between v2 and v3
are in the individual patches.

Thanks
  Dave

Dave Stevenson (4):
  [media] v4l2-common: Add helper function for fourcc to string
  [media] dt-bindings: Document BCM283x CSI2/CCP2 receiver
  [media] bcm2835-unicam: Driver for CCP2/CSI2 camera interface
  MAINTAINERS: Add entry for BCM2835 camera driver

 .../devicetree/bindings/media/bcm2835-unicam.txt   |   85 +
 MAINTAINERS|7 +
 drivers/media/platform/Kconfig |1 +
 drivers/media/platform/Makefile|1 +
 drivers/media/platform/bcm2835/Kconfig |   14 +
 drivers/media/platform/bcm2835/Makefile|3 +
 drivers/media/platform/bcm2835/bcm2835-unicam.c| 2192 
 drivers/media/platform/bcm2835/vc4-regs-unicam.h   |  264 +++
 drivers/media/v4l2-core/v4l2-common.c  |   18 +
 include/media/v4l2-common.h|3 +
 10 files changed, 2588 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/bcm2835-unicam.txt
 create mode 100644 drivers/media/platform/bcm2835/Kconfig
 create mode 100644 drivers/media/platform/bcm2835/Makefile
 create mode 100644 drivers/media/platform/bcm2835/bcm2835-unicam.c
 create mode 100644 drivers/media/platform/bcm2835/vc4-regs-unicam.h

-- 
2.7.4

[PATCH v3 1/4] [media] v4l2-common: Add helper function for fourcc to string

[Dave Stevenson]

New helper function char *v4l2_fourcc2s(u32 fourcc, char *buf)
that converts a fourcc into a nice printable version.

Signed-off-by: Dave Stevenson <dave.steven...@raspberrypi.org>
---

No changes from v2 to v3

 drivers/media/v4l2-core/v4l2-common.c | 18 ++
 include/media/v4l2-common.h   |  3 +++
 2 files changed, 21 insertions(+)

diff --git a/drivers/media/v4l2-core/v4l2-common.c 
b/drivers/media/v4l2-core/v4l2-common.c
index a5ea1f5..0219895 100644
--- a/drivers/media/v4l2-core/v4l2-common.c
+++ b/drivers/media/v4l2-core/v4l2-common.c
@@ -405,3 +405,21 @@ void v4l2_get_timestamp(struct timeval *tv)
tv->tv_usec = ts.tv_nsec / NSEC_PER_USEC;
 }
 EXPORT_SYMBOL_GPL(v4l2_get_timestamp);
+
+char *v4l2_fourcc2s(u32 fourcc, char *buf)
+{
+   buf[0] = fourcc & 0x7f;
+   buf[1] = (fourcc >> 8) & 0x7f;
+   buf[2] = (fourcc >> 16) & 0x7f;
+   buf[3] = (fourcc >> 24) & 0x7f;
+   if (fourcc & (1 << 31)) {
+   buf[4] = '-';
+   buf[5] = 'B';
+   buf[6] = 'E';
+   buf[7] = '\0';
+   } else {
+   buf[4] = '\0';
+   }
+   return buf;
+}
+EXPORT_SYMBOL_GPL(v4l2_fourcc2s);
diff --git a/include/media/v4l2-common.h b/include/media/v4l2-common.h
index aac8b7b..5b0fff9 100644
--- a/include/media/v4l2-common.h
+++ b/include/media/v4l2-common.h
@@ -264,4 +264,7 @@ const struct v4l2_frmsize_discrete 
*v4l2_find_nearest_format(
 
 void v4l2_get_timestamp(struct timeval *tv);
 
+#define V4L2_FOURCC_MAX_SIZE 8
+char *v4l2_fourcc2s(u32 fourcc, char *buf);
+
 #endif /* V4L2_COMMON_H_ */
-- 
2.7.4

Re: [PATCH 2/3] [media] tc358743: Increase FIFO level to 300.

[Dave Stevenson]

On 20 September 2017 at 12:24, Hans Verkuil <hansv...@cisco.com> wrote:
> On 09/20/17 13:00, Dave Stevenson wrote:
>> On 20 September 2017 at 11:23, Philipp Zabel <p.za...@pengutronix.de> wrote:
>>> Hi,
>>>
>>> On Wed, 2017-09-20 at 10:14 +0100, Dave Stevenson wrote:
>>>> Hi Mauro & Philipp
>>>>
>>>> On 19 September 2017 at 17:49, Mauro Carvalho Chehab
>>>> <mche...@s-opensource.com> wrote:
>>>>> Em Tue, 19 Sep 2017 17:24:45 +0200
>>>>> Philipp Zabel <p.za...@pengutronix.de> escreveu:
>>>>>
>>>>>> Hi Dave,
>>>>>>
>>>>>> On Tue, 2017-09-19 at 14:08 +0100, Dave Stevenson wrote:
>>>>>>> The existing fixed value of 16 worked for UYVY 720P60 over
>>>>>>> 2 lanes at 594MHz, or UYVY 1080P60 over 4 lanes. (RGB888
>>>>>>> 1080P60 needs 6 lanes at 594MHz).
>>>>>>> It doesn't allow for lower resolutions to work as the FIFO
>>>>>>> underflows.
>>>>>>>
>>>>>>> Using a value of 300 works for all resolutions down to VGA60,
>>>>>>> and the increase in frame delay is <4usecs for 1080P60 UYVY
>>>>>>> (2.55usecs for RGB888).
>>>>>>>
>>>>>>> Signed-off-by: Dave Stevenson <dave.steven...@raspberrypi.org>
>>>>>>
>>>>>> Can we increase this to 320? This would also allow
>>>>>> 720p60 at 594 Mbps / 4 lanes, according to the xls.
>>>>
>>>> Unless I've missed something then the driver would currently request
>>>> only 2 lanes for 720p60 UYVY, and that works with the existing FIFO
>>>> setting of 16. Likewise 720p60 RGB888 requests 3 lanes and also works
>>>> on a FIFO setting of 16.
>>>> How/why were you thinking we need to run all four lanes for 720p60
>>>> without other significant driver mods around lane config?
>>>
>>> The driver currently silently changes the number of active lanes
>>> depending on required data rate, with no way to communicate it to the
>>> receiver.
>>
>> It is communicated over the subdevice API - tc358743_g_mbus_config
>> reports back the appropriate number of lanes to the receiver
>> subdevice.
>> A suitable v4l2_subdev_has_op(dev->sensor, video, g_mbus_config) call
>> as you're starting streaming therefore gives you the correct
>> information. That's what I've just done for the BCM283x Unicam
>> driver[1], but admittedly I'm not using the media controller API which
>> i.MX6 is.
>
> Shouldn't this information come from the device tree? The g_mbus_config
> op is close to being deprecated or even removed. There are currently only
> two obscure V4L2 bridge drivers that call it. It dates from pre-DT times
> I rather not see it used in a new bridge driver.
>
> The problem is that contains data that belongs to the DT (hardware
> capabilities). Things that can actually change dynamically should be
> communicated via another op. We don't have that, so that should be created.
>
> I've CC-ed Sakari, he is the specialist for such things.

You've reminded me that I asked that same question earlier in the
year, and Sakari had replied -
http://www.spinics.net/lists/linux-media/msg115550.html

Is it specifically device tree related? Just because the lanes are
physically there doesn't necessarily mean they have to be used.

A quick test with the spreadsheet appears to say that 1080p24 UYVY
over 4 lanes at 594Mbps needs a FIFO setting >=480 (the max is 511). I
would anticipate that to be one of the worst situations as we're
dealing with a FIFO underflow herewhen there is a significantly faster
CSI rate than HDMI.
It can't be supported with a 972Mbps link frequency over 4 lanes
(needs >=667), and 2 lanes needs a FIFO setting >=374.

I'll see what numbers fall out of the new spreadsheet for all standard
modes. If there are some modes that can't be supported over 4 lanes
then there is an absolute requirement for communicating the number of
lanes to use.

Seeing as Cisco have kit shipping with this chip and driver, can I ask
how they are managing the choice over number of lanes in use?


As for this patch it sounds like we need to crank the FIFO setting up
to the maximum of 511, and potentially a second patch that removes
g_mbus_config and only reads DT if that is the desired behaviour.

>>
>> [1] http://www.spinics.net/lists/linux-media/msg121813.html, as part
>> of the unicam_start_streaming function.
>>
>>> The i.MX6 MIPI CSI-2 receiver driver can't cope with that, as it always
&

Re: [PATCH 2/3] [media] tc358743: Increase FIFO level to 300.

[Dave Stevenson]

On 20 September 2017 at 11:23, Philipp Zabel <p.za...@pengutronix.de> wrote:
> Hi,
>
> On Wed, 2017-09-20 at 10:14 +0100, Dave Stevenson wrote:
>> Hi Mauro & Philipp
>>
>> On 19 September 2017 at 17:49, Mauro Carvalho Chehab
>> <mche...@s-opensource.com> wrote:
>> > Em Tue, 19 Sep 2017 17:24:45 +0200
>> > Philipp Zabel <p.za...@pengutronix.de> escreveu:
>> >
>> > > Hi Dave,
>> > >
>> > > On Tue, 2017-09-19 at 14:08 +0100, Dave Stevenson wrote:
>> > > > The existing fixed value of 16 worked for UYVY 720P60 over
>> > > > 2 lanes at 594MHz, or UYVY 1080P60 over 4 lanes. (RGB888
>> > > > 1080P60 needs 6 lanes at 594MHz).
>> > > > It doesn't allow for lower resolutions to work as the FIFO
>> > > > underflows.
>> > > >
>> > > > Using a value of 300 works for all resolutions down to VGA60,
>> > > > and the increase in frame delay is <4usecs for 1080P60 UYVY
>> > > > (2.55usecs for RGB888).
>> > > >
>> > > > Signed-off-by: Dave Stevenson <dave.steven...@raspberrypi.org>
>> > >
>> > > Can we increase this to 320? This would also allow
>> > > 720p60 at 594 Mbps / 4 lanes, according to the xls.
>>
>> Unless I've missed something then the driver would currently request
>> only 2 lanes for 720p60 UYVY, and that works with the existing FIFO
>> setting of 16. Likewise 720p60 RGB888 requests 3 lanes and also works
>> on a FIFO setting of 16.
>> How/why were you thinking we need to run all four lanes for 720p60
>> without other significant driver mods around lane config?
>
> The driver currently silently changes the number of active lanes
> depending on required data rate, with no way to communicate it to the
> receiver.

It is communicated over the subdevice API - tc358743_g_mbus_config
reports back the appropriate number of lanes to the receiver
subdevice.
A suitable v4l2_subdev_has_op(dev->sensor, video, g_mbus_config) call
as you're starting streaming therefore gives you the correct
information. That's what I've just done for the BCM283x Unicam
driver[1], but admittedly I'm not using the media controller API which
i.MX6 is.

[1] http://www.spinics.net/lists/linux-media/msg121813.html, as part
of the unicam_start_streaming function.

> The i.MX6 MIPI CSI-2 receiver driver can't cope with that, as it always
> activates all four lanes that are configured in the device tree. I can
> work around that with the following patch:

It can't cope running at less than 4 lanes, or it can't cope with a change?

> --8<--
> Subject: [PATCH] [media] tc358743: do not dynamically reduce number of lanes
>
> Dynamic lane number reduction does not work with receivers that
> configure a fixed lane number according to the device tree settings.
> To allow 720p60 at 594 Mbit/s on 4 lanes, increase the fifo_level
> and tclk_trailcnt settings.
>
> Signed-off-by: Philipp Zabel <p.za...@pengutronix.de>
> ---
>  drivers/media/i2c/tc358743.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/media/i2c/tc358743.c b/drivers/media/i2c/tc358743.c
> index 64f504542a819..70a9435928cdb 100644
> --- a/drivers/media/i2c/tc358743.c
> +++ b/drivers/media/i2c/tc358743.c
> @@ -683,7 +683,7 @@ static void tc358743_set_csi(struct v4l2_subdev *sd)
>  {
> struct tc358743_state *state = to_state(sd);
> struct tc358743_platform_data *pdata = >pdata;
> -   unsigned lanes = tc358743_num_csi_lanes_needed(sd);
> +   unsigned lanes = state->bus.num_data_lanes;
>
> v4l2_dbg(3, debug, sd, "%s:\n", __func__);
>
> @@ -1906,7 +1906,7 @@ static int tc358743_probe_of(struct tc358743_state 
> *state)
> state->pdata.ddc5v_delay = DDC5V_DELAY_100_MS;
> state->pdata.enable_hdcp = false;
> /* A FIFO level of 16 should be enough for 2-lane 720p60 at 594 MHz. 
> */
> -   state->pdata.fifo_level = 16;
> +   state->pdata.fifo_level = 320;
> /*
>  * The PLL input clock is obtained by dividing refclk by pll_prd.
>  * It must be between 6 MHz and 40 MHz, lower frequency is better.
> @@ -1948,7 +1948,7 @@ static int tc358743_probe_of(struct tc358743_state 
> *state)
> state->pdata.lptxtimecnt = 0x003;
> /* tclk-preparecnt: 3, tclk-zerocnt: 20 */
> state->pdata.tclk_headercnt = 0x1403;
> -   state->pdata.tclk_trailcnt = 0x00;
> +   state->pdata.tclk_trailcnt = 0x01;
> /* ths-preparecnt: 3, ths-zerocnt: 1 */
> state->pdata.ths_headercnt 

Re: [PATCH 2/3] [media] tc358743: Increase FIFO level to 300.

[Dave Stevenson]

Hi Mauro & Philipp

On 19 September 2017 at 17:49, Mauro Carvalho Chehab
<mche...@s-opensource.com> wrote:
> Em Tue, 19 Sep 2017 17:24:45 +0200
> Philipp Zabel <p.za...@pengutronix.de> escreveu:
>
>> Hi Dave,
>>
>> On Tue, 2017-09-19 at 14:08 +0100, Dave Stevenson wrote:
>> > The existing fixed value of 16 worked for UYVY 720P60 over
>> > 2 lanes at 594MHz, or UYVY 1080P60 over 4 lanes. (RGB888
>> > 1080P60 needs 6 lanes at 594MHz).
>> > It doesn't allow for lower resolutions to work as the FIFO
>> > underflows.
>> >
>> > Using a value of 300 works for all resolutions down to VGA60,
>> > and the increase in frame delay is <4usecs for 1080P60 UYVY
>> > (2.55usecs for RGB888).
>> >
>> > Signed-off-by: Dave Stevenson <dave.steven...@raspberrypi.org>
>>
>> Can we increase this to 320? This would also allow
>> 720p60 at 594 Mbps / 4 lanes, according to the xls.

Unless I've missed something then the driver would currently request
only 2 lanes for 720p60 UYVY, and that works with the existing FIFO
setting of 16. Likewise 720p60 RGB888 requests 3 lanes and also works
on a FIFO setting of 16.
How/why were you thinking we need to run all four lanes for 720p60
without other significant driver mods around lane config?

Once I've got a v3 done on the Unicam driver I'll bash through the
standard HDMI modes and check what value they need - I can see a big
spreadsheet coming on.
I'll ignore interlaced modes as I can't see any support for it in the
driver. Receiving the fields on different CSI-2 data types is
something I know the Unicam hardware won't handle nicely, and I
suspect it'd be an issue for many other platforms too.

> Hmm... if this is dependent on the resolution and frame rate, wouldn't
> it be better to dynamically adjust it accordingly?

It's setting up the FIFO matching the incoming HDMI data rate and
outgoing CSI rate. That means it's dependent on the incoming pixel
clock, blanking, colour format and resolution, and output CSI link
frequency, number of lanes, and colour format.
Whilst it could be set dynamically based on all those parameters, is
there a significant enough gain in doing so?

The value of 300 works for all cases I've tried, and referencing back
it is also the value that Hans said Cisco use via platform data on
their hardware [1]. Generally I'm seeing that values of 0-130 are
required, so 300 is giving a fair safety margin.

Second question is does anyone have a suitable relationship with
Toshiba to get permission to release details of these register
calculations? The datasheet and value spreadsheet are marked as
confidential, and probably under NDA in almost all cases. Whilst they
can't object to drivers containing values to make them work, they
might over releasing significant details.

Thanks,
  Dave

[1] https://www.spinics.net/lists/linux-media/msg116360.html

Re: [PATCH v2 2/4] [media] dt-bindings: Document BCM283x CSI2/CCP2 receiver

[Dave Stevenson]

Hi Rob.

Thanks for the review.

On 19 September 2017 at 15:32, Rob Herring <r...@kernel.org> wrote:
> On Wed, Sep 13, 2017 at 04:07:47PM +0100, Dave Stevenson wrote:
>> Document the DT bindings for the CSI2/CCP2 receiver peripheral
>> (known as Unicam) on BCM283x SoCs.
>>
>> Signed-off-by: Dave Stevenson <dave.steven...@raspberrypi.org>
>> ---
>>  .../devicetree/bindings/media/bcm2835-unicam.txt   | 107 
>> +
>>  1 file changed, 107 insertions(+)
>>  create mode 100644 
>> Documentation/devicetree/bindings/media/bcm2835-unicam.txt
>>
>> diff --git a/Documentation/devicetree/bindings/media/bcm2835-unicam.txt 
>> b/Documentation/devicetree/bindings/media/bcm2835-unicam.txt
>> new file mode 100644
>> index 000..2ee5af7
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/media/bcm2835-unicam.txt
>> @@ -0,0 +1,107 @@
>> +Broadcom BCM283x Camera Interface (Unicam)
>> +--
>> +
>> +The Unicam block on BCM283x SoCs is the receiver for either
>> +CSI-2 or CCP2 data from image sensors or similar devices.
>> +
>> +There are two camera drivers in the kernel for BCM283x - this one
>> +and bcm2835-camera (currently in staging).
>
> Linux detail that is n/a for bindings.
>
>> +
>> +This driver is purely the kernel controlling the Unicam peripheral - there
>
> Bindings describe h/w blocks, not drivers.
>
>> +is no involvement with the VideoCore firmware. Unicam receives CSI-2
>> +(or CCP2) data and writes it into SDRAM. There is no additional processing
>> +performed.
>> +It should be possible to connect it to any sensor with a
>> +suitable output interface and V4L2 subdevice driver.
>> +
>> +bcm2835-camera uses the VideoCore firmware to control the sensor,
>> +Unicam, ISP, and various tuner control loops. Fully processed frames are
>> +delivered to the driver by the firmware. It only has sensor drivers
>> +for Omnivision OV5647, and Sony IMX219 sensors, and is closed source.
>> +
>> +The two drivers are mutually exclusive for the same Unicam instance.
>> +The firmware checks the device tree configuration during boot. If
>> +it finds device tree nodes called csi0 or csi1 then it will block the
>> +firmware from accessing the peripheral, and bcm2835-camera will
>> +not be able to stream data.
>
> All interesting, but irrelavent to the binding other than the part about
> node name. Reword to just state the requirements to get the firmware to
> ignore things.

Hans had suggested it was potentially appropriate for DT bindings too
on last review [1] (which I apologise for missing off devicetree folk
from), but I'll remove all mention of Linux/driver detail for v3.
That will leave only a reworded version of the bit about making the
firmware ignore the blocks.

[1] http://www.spinics.net/lists/linux-media/msg117047.html

>> +It should be possible to use bcm2835-camera on one camera interface
>> +and bcm2835-unicam on the other interface if there is a need to.
>
> For upstream, I don't think we care to support that. We don't need 2
> bindings.

I'll remove as irrelevant, but in that case wouldn't you have 2
bindings - one to bcm2835-camera which references the vchi node to the
GPU, and one to bcm2835-unicam that gives the full block config as
being discussed in this doc.
(Currently bcm2835-camera is a platform device so has no binding doc).
This probably isn't the place for such a discussion.

>> +
>> +Required properties:
>> +===
>> +- compatible : must be "brcm,bcm2835-unicam".
>> +- reg: physical base address and length of the register 
>> sets for the
>> +   device.
>> +- interrupts : should contain the IRQ line for this Unicam instance.
>> +- clocks : list of clock specifiers, corresponding to entries in
>> +   clock-names property.
>> +- clock-names: must contain an "lp_clock" entry, matching entries
>> +   in the clocks property.
>
> _clock is redundant.

So just "lp" as the clock name? Will assume so.

>> +
>> +Unicam supports a single port node. It should contain one 'port' child node
>> +with child 'endpoint' node. Please refer to the bindings defined in
>> +Documentation/devicetree/bindings/media/video-interfaces.txt.
>> +
>> +Within the endpoint node, the following properties are mandatory:
>> +- remote-endpoint: links to the source device endpoint.
>> +- data-lanes : An array denoting how many data lanes are physically
>> +   present for this CSI

[PATCH 0/3] [media] tc358743: Support for a wider range of inputs

[Dave Stevenson]

Three minor changes to the TC358743 HDMI to CSI2 bridge chip driver.
- Correct the clock mode reported via g_mbus_config to match that set.
- Increase the FIFO level to allow resolutions lower than 720P60 to work.
- Add settings for a new link frequency of 972Mbit/s. This allows
  1080P50 UYVY to work on two lanes (useful on the Raspberry Pi which
  only brings out two CSI2 lanes to the camera connector).

I'd like to extend the last one to dynamically calculate all the values
for an arbitrary link speed, but time hasn't allowed as yet.

Dave Stevenson (3):
  [media] tc358743: Correct clock mode reported in g_mbus_config
  [media] tc358743: Increase FIFO level to 300.
  [media] tc358743: Add support for 972Mbit/s link freq.

 drivers/media/i2c/tc358743.c | 62 +++-
 1 file changed, 44 insertions(+), 18 deletions(-)

-- 
2.7.4

[PATCH 3/3] [media] tc358743: Add support for 972Mbit/s link freq.

[Dave Stevenson]

Adds register setups for running the CSI lanes at 972Mbit/s,
which allows 1080P50 UYVY down 2 lanes.

Signed-off-by: Dave Stevenson <dave.steven...@raspberrypi.org>
---
 drivers/media/i2c/tc358743.c | 47 +++-
 1 file changed, 33 insertions(+), 14 deletions(-)

diff --git a/drivers/media/i2c/tc358743.c b/drivers/media/i2c/tc358743.c
index 7632daf..dcc100e 100644
--- a/drivers/media/i2c/tc358743.c
+++ b/drivers/media/i2c/tc358743.c
@@ -1809,6 +1809,7 @@ static int tc358743_probe_of(struct tc358743_state *state)
/*
 * The CSI bps per lane must be between 62.5 Mbps and 1 Gbps.
 * The default is 594 Mbps for 4-lane 1080p60 or 2-lane 720p60.
+* 972 Mbps allows 1080P50 UYVY over 2-lane.
 */
bps_pr_lane = 2 * endpoint->link_frequencies[0];
if (bps_pr_lane < 6250U || bps_pr_lane > 10U) {
@@ -1821,23 +1822,41 @@ static int tc358743_probe_of(struct tc358743_state 
*state)
   state->pdata.refclk_hz * state->pdata.pll_prd;
 
/*
-* FIXME: These timings are from REF_02 for 594 Mbps per lane (297 MHz
-* link frequency). In principle it should be possible to calculate
+* FIXME: These timings are from REF_02 for 594 or 972 Mbps per lane
+* (297 MHz or 495 MHz link frequency).
+* In principle it should be possible to calculate
 * them based on link frequency and resolution.
 */
-   if (bps_pr_lane != 59400U)
+   switch (bps_pr_lane) {
+   default:
dev_warn(dev, "untested bps per lane: %u bps\n", bps_pr_lane);
-   state->pdata.lineinitcnt = 0xe80;
-   state->pdata.lptxtimecnt = 0x003;
-   /* tclk-preparecnt: 3, tclk-zerocnt: 20 */
-   state->pdata.tclk_headercnt = 0x1403;
-   state->pdata.tclk_trailcnt = 0x00;
-   /* ths-preparecnt: 3, ths-zerocnt: 1 */
-   state->pdata.ths_headercnt = 0x0103;
-   state->pdata.twakeup = 0x4882;
-   state->pdata.tclk_postcnt = 0x008;
-   state->pdata.ths_trailcnt = 0x2;
-   state->pdata.hstxvregcnt = 0;
+   case 59400U:
+   state->pdata.lineinitcnt = 0xe80;
+   state->pdata.lptxtimecnt = 0x003;
+   /* tclk-preparecnt: 3, tclk-zerocnt: 20 */
+   state->pdata.tclk_headercnt = 0x1403;
+   state->pdata.tclk_trailcnt = 0x00;
+   /* ths-preparecnt: 3, ths-zerocnt: 1 */
+   state->pdata.ths_headercnt = 0x0103;
+   state->pdata.twakeup = 0x4882;
+   state->pdata.tclk_postcnt = 0x008;
+   state->pdata.ths_trailcnt = 0x2;
+   state->pdata.hstxvregcnt = 0;
+   break;
+   case 97200U:
+   state->pdata.lineinitcnt = 0xFA0;
+   state->pdata.lptxtimecnt = 0x007;
+   /* tclk-preparecnt: 6, tclk-zerocnt: 40 */
+   state->pdata.tclk_headercnt = 0x2806;
+   state->pdata.tclk_trailcnt = 0x00;
+   /* ths-preparecnt: 3, ths-zerocnt: 1 */
+   state->pdata.ths_headercnt = 0x0806;
+   state->pdata.twakeup = 0x4268;
+   state->pdata.tclk_postcnt = 0x008;
+   state->pdata.ths_trailcnt = 0x5;
+   state->pdata.hstxvregcnt = 0;
+   break;
+   }
 
state->reset_gpio = devm_gpiod_get_optional(dev, "reset",
GPIOD_OUT_LOW);
-- 
2.7.4

[PATCH 1/3] [media] tc358743: Correct clock mode reported in g_mbus_config

[Dave Stevenson]

Support for non-continuous clock had previously been added via
device tree, but a comment and the value reported by g_mbus_config
still stated that it wasn't supported.
Remove the comment, and return the correct value in g_mbus_config.

Signed-off-by: Dave Stevenson <dave.steven...@raspberrypi.org>
---
 drivers/media/i2c/tc358743.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/media/i2c/tc358743.c b/drivers/media/i2c/tc358743.c
index e6f5c36..6b0fd07 100644
--- a/drivers/media/i2c/tc358743.c
+++ b/drivers/media/i2c/tc358743.c
@@ -1461,8 +1461,9 @@ static int tc358743_g_mbus_config(struct v4l2_subdev *sd,
 
cfg->type = V4L2_MBUS_CSI2;
 
-   /* Support for non-continuous CSI-2 clock is missing in the driver */
-   cfg->flags = V4L2_MBUS_CSI2_CONTINUOUS_CLOCK;
+   cfg->flags = state->bus.flags &
+   (V4L2_MBUS_CSI2_CONTINUOUS_CLOCK |
+V4L2_MBUS_CSI2_NONCONTINUOUS_CLOCK);
 
switch (state->csi_lanes_in_use) {
case 1:
-- 
2.7.4

[PATCH 2/3] [media] tc358743: Increase FIFO level to 300.

[Dave Stevenson]

The existing fixed value of 16 worked for UYVY 720P60 over
2 lanes at 594MHz, or UYVY 1080P60 over 4 lanes. (RGB888
1080P60 needs 6 lanes at 594MHz).
It doesn't allow for lower resolutions to work as the FIFO
underflows.

Using a value of 300 works for all resolutions down to VGA60,
and the increase in frame delay is <4usecs for 1080P60 UYVY
(2.55usecs for RGB888).

Signed-off-by: Dave Stevenson <dave.steven...@raspberrypi.org>
---
 drivers/media/i2c/tc358743.c | 10 --
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/media/i2c/tc358743.c b/drivers/media/i2c/tc358743.c
index 6b0fd07..7632daf 100644
--- a/drivers/media/i2c/tc358743.c
+++ b/drivers/media/i2c/tc358743.c
@@ -1782,8 +1782,14 @@ static int tc358743_probe_of(struct tc358743_state 
*state)
state->pdata.refclk_hz = clk_get_rate(refclk);
state->pdata.ddc5v_delay = DDC5V_DELAY_100_MS;
state->pdata.enable_hdcp = false;
-   /* A FIFO level of 16 should be enough for 2-lane 720p60 at 594 MHz. */
-   state->pdata.fifo_level = 16;
+   /*
+* A FIFO level of 16 should be enough for 2-lane 720p60 at 594 MHz,
+* but is insufficient for lower resolutions.
+* A value of 300 allows for resolutions down to VGA60 (and possibly
+* lower) to work, whilst still leaving the delay for 1080P60
+* stilll below 4usecs.
+*/
+   state->pdata.fifo_level = 300;
/*
 * The PLL input clock is obtained by dividing refclk by pll_prd.
 * It must be between 6 MHz and 40 MHz, lower frequency is better.
-- 
2.7.4

Re: [PATCH v2 3/4] [media] bcm2835-unicam: Driver for CCP2/CSI2 camera interface

[Dave Stevenson]

Hi Hans.

Thanks for the response.

On 19 September 2017 at 11:20, Hans Verkuil <hverk...@xs4all.nl> wrote:
> On 09/19/17 11:50, Dave Stevenson wrote:
>> Hi Eric.
>>
>> Thanks for the review.
>>
>> On 18 September 2017 at 19:18, Eric Anholt <e...@anholt.net> wrote:
>>> Dave Stevenson <dave.steven...@raspberrypi.org> writes:
>>>> diff --git a/drivers/media/platform/bcm2835/bcm2835-unicam.c 
>>>> b/drivers/media/platform/bcm2835/bcm2835-unicam.c
>>>> new file mode 100644
>>>> index 000..5b1adc3
>>>> --- /dev/null
>>>> +++ b/drivers/media/platform/bcm2835/bcm2835-unicam.c
>>>> @@ -0,0 +1,2192 @@
>>>> +/*
>>>> + * BCM2835 Unicam capture Driver
>>>> + *
>>>> + * Copyright (C) 2017 - Raspberry Pi (Trading) Ltd.
>>>> + *
>>>> + * Dave Stevenson <dave.steven...@raspberrypi.org>
>>>> + *
>>>> + * Based on TI am437x driver by Benoit Parrot and Lad, Prabhakar and
>>>> + * TI CAL camera interface driver by Benoit Parrot.
>>>> + *
>>>
>>> Possible future improvement: this description of the driver is really
>>> nice and could be turned into kernel-doc.
>>
>> Documentation?! Surely not :-)
>> For now I'll leave it as a task for another day.
>>
>>>> + * There are two camera drivers in the kernel for BCM283x - this one
>>>> + * and bcm2835-camera (currently in staging).
>>>> + *
>>>> + * This driver is purely the kernel control the Unicam peripheral - there
>>>
>>> Maybe "This driver directly controls..."?
>>
>> Will do in v3.
>>
>>>> + * is no involvement with the VideoCore firmware. Unicam receives CSI-2
>>>> + * or CCP2 data and writes it into SDRAM. The only processing options are
>>>> + * to repack Bayer data into an alternate format, and applying windowing
>>>> + * (currently not implemented).
>>>> + * It should be possible to connect it to any sensor with a
>>>> + * suitable output interface and V4L2 subdevice driver.
>>>> + *
>>>> + * bcm2835-camera uses with the VideoCore firmware to control the sensor,
>>>
>>> "uses the"
>>
>> Will do in v3.
>>
>>>> + * Unicam, ISP, and all tuner control loops. Fully processed frames are
>>>> + * delivered to the driver by the firmware. It only has sensor drivers
>>>> + * for Omnivision OV5647, and Sony IMX219 sensors.
>>>> + *
>>>> + * The two drivers are mutually exclusive for the same Unicam instance.
>>>> + * The VideoCore firmware checks the device tree configuration during 
>>>> boot.
>>>> + * If it finds device tree nodes called csi0 or csi1 it will block the
>>>> + * firmware from accessing the peripheral, and bcm2835-camera will
>>>> + * not be able to stream data.
>>>
>>> Thanks for describing this here!
>>>
>>>> +/*
>>>> + * The peripheral can unpack and repack between several of
>>>> + * the Bayer raw formats, so any Bayer format can be advertised
>>>> + * as the same Bayer order in each of the supported bit depths.
>>>> + * Use lower case to avoid clashing with V4L2_PIX_FMT_SGBRG8
>>>> + * formats.
>>>> + */
>>>> +#define PIX_FMT_ALL_BGGR  v4l2_fourcc('b', 'g', 'g', 'r')
>>>> +#define PIX_FMT_ALL_RGGB  v4l2_fourcc('r', 'g', 'g', 'b')
>>>> +#define PIX_FMT_ALL_GBRG  v4l2_fourcc('g', 'b', 'r', 'g')
>>>> +#define PIX_FMT_ALL_GRBG  v4l2_fourcc('g', 'r', 'b', 'g')
>>>
>>> Should thes fourccs be defined in a common v4l2 header, to reserve it
>>> from clashing with others later?
>>
>> I'm only using them as flags and probably in a manner that nothing
>> else is likely to copy, so it seems a little excessive to put them in
>> a common header.
>> Perhaps it's better to switch to 0xFFF0 to 0xFFF3 or other
>> value that won't come up as a fourcc under any normal circumstance.
>> Any thoughts from other people?
>
> I think that's better, yes.

OK, happy to do that.

>>
>>> This is really the only question I have about this driver before seeing
>>> it merged.  As far as me wearing my platform maintainer hat, I'm happy
>>> with the driver, and my other little notes are optional.
>>>
>>>> +static int unicam_probe(struct platform_device *pdev)
>>>> +{
>>>> + struct unicam_cfg *unicam_

Re: [PATCH v2 3/4] [media] bcm2835-unicam: Driver for CCP2/CSI2 camera interface

[Dave Stevenson]

Hi Eric.

Thanks for the review.

On 18 September 2017 at 19:18, Eric Anholt <e...@anholt.net> wrote:
> Dave Stevenson <dave.steven...@raspberrypi.org> writes:
>> diff --git a/drivers/media/platform/bcm2835/bcm2835-unicam.c 
>> b/drivers/media/platform/bcm2835/bcm2835-unicam.c
>> new file mode 100644
>> index 000..5b1adc3
>> --- /dev/null
>> +++ b/drivers/media/platform/bcm2835/bcm2835-unicam.c
>> @@ -0,0 +1,2192 @@
>> +/*
>> + * BCM2835 Unicam capture Driver
>> + *
>> + * Copyright (C) 2017 - Raspberry Pi (Trading) Ltd.
>> + *
>> + * Dave Stevenson <dave.steven...@raspberrypi.org>
>> + *
>> + * Based on TI am437x driver by Benoit Parrot and Lad, Prabhakar and
>> + * TI CAL camera interface driver by Benoit Parrot.
>> + *
>
> Possible future improvement: this description of the driver is really
> nice and could be turned into kernel-doc.

Documentation?! Surely not :-)
For now I'll leave it as a task for another day.

>> + * There are two camera drivers in the kernel for BCM283x - this one
>> + * and bcm2835-camera (currently in staging).
>> + *
>> + * This driver is purely the kernel control the Unicam peripheral - there
>
> Maybe "This driver directly controls..."?

Will do in v3.

>> + * is no involvement with the VideoCore firmware. Unicam receives CSI-2
>> + * or CCP2 data and writes it into SDRAM. The only processing options are
>> + * to repack Bayer data into an alternate format, and applying windowing
>> + * (currently not implemented).
>> + * It should be possible to connect it to any sensor with a
>> + * suitable output interface and V4L2 subdevice driver.
>> + *
>> + * bcm2835-camera uses with the VideoCore firmware to control the sensor,
>
> "uses the"

Will do in v3.

>> + * Unicam, ISP, and all tuner control loops. Fully processed frames are
>> + * delivered to the driver by the firmware. It only has sensor drivers
>> + * for Omnivision OV5647, and Sony IMX219 sensors.
>> + *
>> + * The two drivers are mutually exclusive for the same Unicam instance.
>> + * The VideoCore firmware checks the device tree configuration during boot.
>> + * If it finds device tree nodes called csi0 or csi1 it will block the
>> + * firmware from accessing the peripheral, and bcm2835-camera will
>> + * not be able to stream data.
>
> Thanks for describing this here!
>
>> +/*
>> + * The peripheral can unpack and repack between several of
>> + * the Bayer raw formats, so any Bayer format can be advertised
>> + * as the same Bayer order in each of the supported bit depths.
>> + * Use lower case to avoid clashing with V4L2_PIX_FMT_SGBRG8
>> + * formats.
>> + */
>> +#define PIX_FMT_ALL_BGGR  v4l2_fourcc('b', 'g', 'g', 'r')
>> +#define PIX_FMT_ALL_RGGB  v4l2_fourcc('r', 'g', 'g', 'b')
>> +#define PIX_FMT_ALL_GBRG  v4l2_fourcc('g', 'b', 'r', 'g')
>> +#define PIX_FMT_ALL_GRBG  v4l2_fourcc('g', 'r', 'b', 'g')
>
> Should thes fourccs be defined in a common v4l2 header, to reserve it
> from clashing with others later?

I'm only using them as flags and probably in a manner that nothing
else is likely to copy, so it seems a little excessive to put them in
a common header.
Perhaps it's better to switch to 0xFFF0 to 0xFFF3 or other
value that won't come up as a fourcc under any normal circumstance.
Any thoughts from other people?

> This is really the only question I have about this driver before seeing
> it merged.  As far as me wearing my platform maintainer hat, I'm happy
> with the driver, and my other little notes are optional.
>
>> +static int unicam_probe(struct platform_device *pdev)
>> +{
>> + struct unicam_cfg *unicam_cfg;
>> + struct unicam_device *unicam;
>> + struct v4l2_ctrl_handler *hdl;
>> + struct resource *res;
>> + int ret;
>> +
>> + unicam = devm_kzalloc(>dev, sizeof(*unicam), GFP_KERNEL);
>> + if (!unicam)
>> + return -ENOMEM;
>> +
>> + unicam->pdev = pdev;
>> + unicam_cfg = >cfg;
>> +
>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> + unicam_cfg->base = devm_ioremap_resource(>dev, res);
>> + if (IS_ERR(unicam_cfg->base)) {
>> + unicam_err(unicam, "Failed to get main io block\n");
>> + return PTR_ERR(unicam_cfg->base);
>> + }
>> +
>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
>> + unicam_cfg->clk_gate_base = devm_ioremap_resource(>dev, res);
>> + if (IS_ERR(unicam_cfg->clk_gate_base)) {
>

Re: [PATCH v2 0/4] BCM283x Camera Receiver driver

[Dave Stevenson]

, Failed: 1, Warnings: 0


Hans previously requested the output of "v4l2-ctl -l" for this case:
pi@raspberrypi:~/v4l-utils/utils/v4l2-ctl $ ./v4l2-ctl -l
pi@raspberrypi:~/v4l-utils/utils/v4l2-ctl $
ie nothing - the sub device driver has no controls registered, and
that is what causes the failure:
fail: v4l2-test-controls.cpp(587): g_ext_ctrls does not
support count == 0
test VIDIOC_G/S/TRY_EXT_CTRLS: FAIL
I don't know what the correct behaviour should be in those
circumstances, but it isn't really a failure of this driver.

  Dave

On 13 September 2017 at 16:07, Dave Stevenson
<dave.steven...@raspberrypi.org> wrote:
> Hi All.
>
> This is v2 for adding a V4L2 subdevice driver for the CSI2/CCP2 camera
> receiver peripheral on BCM283x, as used on Raspberry Pi.
> Sorry for the delay since v1 - other tasks assigned, got sucked
> into investigating why some devices were misbehaving, and
> picking up on new features that had been added to the tree (eg CCP2).
>
> v4l2-compliance results depend on the sensor subdevice connected to.
> I have results for TC358743, ADV7282M, and OV5647 that I'll
> send them as a follow up email.
>
> OV647 and ADV7282M are now working with this driver, as well as TC358743.
> v1 of the driver only supported continuous clock mode which Unicam was
> failing to lock on to correctly.
> The driver now checks the clock mode and adjusts termination accordingly.
> Something is still a little off for OV5647, but I'll investigate that
> later.
>
> As per the v1 discussion with Hans, I have added text describing the
> differences between this driver and the one in staging/vc04_service.
> Addressing some of the issues in the bcm2835-camera driver is on my to-do
> list, and I'll add similar text there when I'm dealing with that.
>
> For those wanting to see the driver in context,
> https://github.com/6by9/upstream-linux/tree/unicam is the linux-media
> tree with my mods on top. It also includes a couple of TC358743 and
> OV5647 driver updates that I'll send to the list in the next few days.
>
> Thanks in advance.
>   Dave
>
> Changes from v1 to v2:
> - Broken out a new helper function v4l2_fourcc2s as requested by Hans.
> - Documented difference between this driver and the bcm2835-camera driver
>   in staging/vc04_services.
> - Corrected handling of s_dv_timings and s_std to update the current format
>   but only if not streaming. This refactored some of the s_fmt code to
>   remove duplication.
> - Updated handling of sizeimage to include vertical padding. (Not updated
>   the bytesperline calcs as the app can override).
> - Added support for continuous clock mode (requires changes to lane
>   termination configuration).
> - Add support for CCP2 as Sakari's patches to support it have now been merged.
>   I don't have a suitable sensor to test it with at present, but all settings
>   have been taken from a known working configuration. If people would prefer
>   I remove this until it has been proved against hardware then I'm happy to
>   do so.
> - Updated DT bindings to use  on the Unicam node to set the
>   maximum number of lanes present instead of a having a custom property.
>   Documents the mandatory endpoint properties.
> - Removed RAW16 from the list of input formats as it isn't defined in the
>   CSI-2 spec. The peripheral can still unpack the other Bayer formats to
>   a 16 bit/pixel packing though.
> - Added a log-status handler to get the status from the sensor.
> - Automatically switch away from any interlaced formats reported via g_fmt,
>   or that are attempted to be set via try/s_fmt.
> - Addressed other more minor code review comments from v1.
>
> Dave Stevenson (4):
>   [media] v4l2-common: Add helper function for fourcc to string
>   [media] dt-bindings: Document BCM283x CSI2/CCP2 receiver
>   [media] bcm2835-unicam: Driver for CCP2/CSI2 camera interface
>   MAINTAINERS: Add entry for BCM2835 camera driver
>
>  .../devicetree/bindings/media/bcm2835-unicam.txt   |  107 +
>  MAINTAINERS|7 +
>  drivers/media/platform/Kconfig |1 +
>  drivers/media/platform/Makefile|1 +
>  drivers/media/platform/bcm2835/Kconfig |   14 +
>  drivers/media/platform/bcm2835/Makefile|3 +
>  drivers/media/platform/bcm2835/bcm2835-unicam.c| 2192 
> 
>  drivers/media/platform/bcm2835/vc4-regs-unicam.h   |  264 +++
>  drivers/media/v4l2-core/v4l2-common.c  |   18 +
>  include/media/v4l2-common.h|3 +
>  10 files changed, 2610 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/bcm2835-unicam.txt
>  create mode 100644 drivers/media/platform/bcm2835/Kconfig
>  create mode 100644 drivers/media/platform/bcm2835/Makefile
>  create mode 100644 drivers/media/platform/bcm2835/bcm2835-unicam.c
>  create mode 100644 drivers/media/platform/bcm2835/vc4-regs-unicam.h
>
> --
> 2.7.4
>

[PATCH v2 3/4] [media] bcm2835-unicam: Driver for CCP2/CSI2 camera interface

[Dave Stevenson]

Add driver for the Unicam camera receiver block on
BCM283x processors.

Signed-off-by: Dave Stevenson <dave.steven...@raspberrypi.org>
---
 drivers/media/platform/Kconfig   |1 +
 drivers/media/platform/Makefile  |1 +
 drivers/media/platform/bcm2835/Kconfig   |   14 +
 drivers/media/platform/bcm2835/Makefile  |3 +
 drivers/media/platform/bcm2835/bcm2835-unicam.c  | 2192 ++
 drivers/media/platform/bcm2835/vc4-regs-unicam.h |  264 +++
 6 files changed, 2475 insertions(+)
 create mode 100644 drivers/media/platform/bcm2835/Kconfig
 create mode 100644 drivers/media/platform/bcm2835/Makefile
 create mode 100644 drivers/media/platform/bcm2835/bcm2835-unicam.c
 create mode 100644 drivers/media/platform/bcm2835/vc4-regs-unicam.h

diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
index 7e7cc49..1e5f004 100644
--- a/drivers/media/platform/Kconfig
+++ b/drivers/media/platform/Kconfig
@@ -150,6 +150,7 @@ source "drivers/media/platform/am437x/Kconfig"
 source "drivers/media/platform/xilinx/Kconfig"
 source "drivers/media/platform/rcar-vin/Kconfig"
 source "drivers/media/platform/atmel/Kconfig"
+source "drivers/media/platform/bcm2835/Kconfig"
 
 config VIDEO_TI_CAL
tristate "TI CAL (Camera Adaptation Layer) driver"
diff --git a/drivers/media/platform/Makefile b/drivers/media/platform/Makefile
index c1ef946..045de3f 100644
--- a/drivers/media/platform/Makefile
+++ b/drivers/media/platform/Makefile
@@ -90,3 +90,4 @@ obj-$(CONFIG_VIDEO_QCOM_CAMSS)+= 
qcom/camss-8x16/
 obj-$(CONFIG_VIDEO_QCOM_VENUS) += qcom/venus/
 
 obj-y  += meson/
+obj-y  += bcm2835/
diff --git a/drivers/media/platform/bcm2835/Kconfig 
b/drivers/media/platform/bcm2835/Kconfig
new file mode 100644
index 000..6a74842
--- /dev/null
+++ b/drivers/media/platform/bcm2835/Kconfig
@@ -0,0 +1,14 @@
+# Broadcom VideoCore4 V4L2 camera support
+
+config VIDEO_BCM2835_UNICAM
+   tristate "Broadcom BCM2835 Unicam video capture driver"
+   depends on VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API
+   depends on ARCH_BCM2835 || COMPILE_TEST
+   select VIDEOBUF2_DMA_CONTIG
+   select V4L2_FWNODE
+   ---help---
+ Say Y here to enable V4L2 subdevice for CSI2 receiver.
+ This is a V4L2 subdevice that interfaces directly to the VC4 
peripheral.
+
+  To compile this driver as a module, choose M here. The module
+  will be called bcm2835-unicam.
diff --git a/drivers/media/platform/bcm2835/Makefile 
b/drivers/media/platform/bcm2835/Makefile
new file mode 100644
index 000..a98aba0
--- /dev/null
+++ b/drivers/media/platform/bcm2835/Makefile
@@ -0,0 +1,3 @@
+# Makefile for BCM2835 Unicam driver
+
+obj-$(CONFIG_VIDEO_BCM2835_UNICAM) += bcm2835-unicam.o
diff --git a/drivers/media/platform/bcm2835/bcm2835-unicam.c 
b/drivers/media/platform/bcm2835/bcm2835-unicam.c
new file mode 100644
index 000..5b1adc3
--- /dev/null
+++ b/drivers/media/platform/bcm2835/bcm2835-unicam.c
@@ -0,0 +1,2192 @@
+/*
+ * BCM2835 Unicam capture Driver
+ *
+ * Copyright (C) 2017 - Raspberry Pi (Trading) Ltd.
+ *
+ * Dave Stevenson <dave.steven...@raspberrypi.org>
+ *
+ * Based on TI am437x driver by Benoit Parrot and Lad, Prabhakar and
+ * TI CAL camera interface driver by Benoit Parrot.
+ *
+ *
+ * There are two camera drivers in the kernel for BCM283x - this one
+ * and bcm2835-camera (currently in staging).
+ *
+ * This driver is purely the kernel control the Unicam peripheral - there
+ * is no involvement with the VideoCore firmware. Unicam receives CSI-2
+ * or CCP2 data and writes it into SDRAM. The only processing options are
+ * to repack Bayer data into an alternate format, and applying windowing
+ * (currently not implemented).
+ * It should be possible to connect it to any sensor with a
+ * suitable output interface and V4L2 subdevice driver.
+ *
+ * bcm2835-camera uses with the VideoCore firmware to control the sensor,
+ * Unicam, ISP, and all tuner control loops. Fully processed frames are
+ * delivered to the driver by the firmware. It only has sensor drivers
+ * for Omnivision OV5647, and Sony IMX219 sensors.
+ *
+ * The two drivers are mutually exclusive for the same Unicam instance.
+ * The VideoCore firmware checks the device tree configuration during boot.
+ * If it finds device tree nodes called csi0 or csi1 it will block the
+ * firmware from accessing the peripheral, and bcm2835-camera will
+ * not be able to stream data.
+ *
+ *
+ * This program is free software; you may redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; version 2 of the License.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
+ * EXPRESS OR IMPLIED, 

[PATCH v2 4/4] MAINTAINERS: Add entry for BCM2835 camera driver

[Dave Stevenson]

Signed-off-by: Dave Stevenson <dave.steven...@raspberrypi.org>
---
 MAINTAINERS | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index eb930eb..b47ddaa 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2713,6 +2713,13 @@ S:   Maintained
 N: bcm2835
 F: drivers/staging/vc04_services
 
+BROADCOM BCM2835 CAMERA DRIVER
+M:     Dave Stevenson <dave.steven...@raspberrypi.org>
+L: linux-media@vger.kernel.org
+S: Maintained
+F: drivers/media/platform/bcm2835/
+F: Documentation/devicetree/bindings/media/bcm2835-unicam.txt
+
 BROADCOM BCM47XX MIPS ARCHITECTURE
 M: Hauke Mehrtens <ha...@hauke-m.de>
 M: Rafał Miłecki <zaj...@gmail.com>
-- 
2.7.4

[PATCH v2 0/4] BCM283x Camera Receiver driver

[Dave Stevenson]

Hi All.

This is v2 for adding a V4L2 subdevice driver for the CSI2/CCP2 camera
receiver peripheral on BCM283x, as used on Raspberry Pi.
Sorry for the delay since v1 - other tasks assigned, got sucked
into investigating why some devices were misbehaving, and
picking up on new features that had been added to the tree (eg CCP2).

v4l2-compliance results depend on the sensor subdevice connected to.
I have results for TC358743, ADV7282M, and OV5647 that I'll
send them as a follow up email.

OV647 and ADV7282M are now working with this driver, as well as TC358743.
v1 of the driver only supported continuous clock mode which Unicam was
failing to lock on to correctly.
The driver now checks the clock mode and adjusts termination accordingly.
Something is still a little off for OV5647, but I'll investigate that
later.

As per the v1 discussion with Hans, I have added text describing the
differences between this driver and the one in staging/vc04_service.
Addressing some of the issues in the bcm2835-camera driver is on my to-do
list, and I'll add similar text there when I'm dealing with that.

For those wanting to see the driver in context,
https://github.com/6by9/upstream-linux/tree/unicam is the linux-media
tree with my mods on top. It also includes a couple of TC358743 and
OV5647 driver updates that I'll send to the list in the next few days.

Thanks in advance.
  Dave

Changes from v1 to v2:
- Broken out a new helper function v4l2_fourcc2s as requested by Hans.
- Documented difference between this driver and the bcm2835-camera driver
  in staging/vc04_services.
- Corrected handling of s_dv_timings and s_std to update the current format
  but only if not streaming. This refactored some of the s_fmt code to
  remove duplication.
- Updated handling of sizeimage to include vertical padding. (Not updated
  the bytesperline calcs as the app can override).
- Added support for continuous clock mode (requires changes to lane
  termination configuration).
- Add support for CCP2 as Sakari's patches to support it have now been merged.
  I don't have a suitable sensor to test it with at present, but all settings
  have been taken from a known working configuration. If people would prefer
  I remove this until it has been proved against hardware then I'm happy to
  do so.
- Updated DT bindings to use  on the Unicam node to set the
  maximum number of lanes present instead of a having a custom property.
  Documents the mandatory endpoint properties.
- Removed RAW16 from the list of input formats as it isn't defined in the
  CSI-2 spec. The peripheral can still unpack the other Bayer formats to
  a 16 bit/pixel packing though.
- Added a log-status handler to get the status from the sensor.
- Automatically switch away from any interlaced formats reported via g_fmt,
  or that are attempted to be set via try/s_fmt.
- Addressed other more minor code review comments from v1.

Dave Stevenson (4):
  [media] v4l2-common: Add helper function for fourcc to string
  [media] dt-bindings: Document BCM283x CSI2/CCP2 receiver
  [media] bcm2835-unicam: Driver for CCP2/CSI2 camera interface
  MAINTAINERS: Add entry for BCM2835 camera driver

 .../devicetree/bindings/media/bcm2835-unicam.txt   |  107 +
 MAINTAINERS|7 +
 drivers/media/platform/Kconfig |1 +
 drivers/media/platform/Makefile|1 +
 drivers/media/platform/bcm2835/Kconfig |   14 +
 drivers/media/platform/bcm2835/Makefile|3 +
 drivers/media/platform/bcm2835/bcm2835-unicam.c| 2192 
 drivers/media/platform/bcm2835/vc4-regs-unicam.h   |  264 +++
 drivers/media/v4l2-core/v4l2-common.c  |   18 +
 include/media/v4l2-common.h|3 +
 10 files changed, 2610 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/bcm2835-unicam.txt
 create mode 100644 drivers/media/platform/bcm2835/Kconfig
 create mode 100644 drivers/media/platform/bcm2835/Makefile
 create mode 100644 drivers/media/platform/bcm2835/bcm2835-unicam.c
 create mode 100644 drivers/media/platform/bcm2835/vc4-regs-unicam.h

-- 
2.7.4

[PATCH v2 2/4] [media] dt-bindings: Document BCM283x CSI2/CCP2 receiver

[Dave Stevenson]

Document the DT bindings for the CSI2/CCP2 receiver peripheral
(known as Unicam) on BCM283x SoCs.

Signed-off-by: Dave Stevenson <dave.steven...@raspberrypi.org>
---
 .../devicetree/bindings/media/bcm2835-unicam.txt   | 107 +
 1 file changed, 107 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/bcm2835-unicam.txt

diff --git a/Documentation/devicetree/bindings/media/bcm2835-unicam.txt 
b/Documentation/devicetree/bindings/media/bcm2835-unicam.txt
new file mode 100644
index 000..2ee5af7
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/bcm2835-unicam.txt
@@ -0,0 +1,107 @@
+Broadcom BCM283x Camera Interface (Unicam)
+--
+
+The Unicam block on BCM283x SoCs is the receiver for either
+CSI-2 or CCP2 data from image sensors or similar devices.
+
+There are two camera drivers in the kernel for BCM283x - this one
+and bcm2835-camera (currently in staging).
+
+This driver is purely the kernel controlling the Unicam peripheral - there
+is no involvement with the VideoCore firmware. Unicam receives CSI-2
+(or CCP2) data and writes it into SDRAM. There is no additional processing
+performed.
+It should be possible to connect it to any sensor with a
+suitable output interface and V4L2 subdevice driver.
+
+bcm2835-camera uses the VideoCore firmware to control the sensor,
+Unicam, ISP, and various tuner control loops. Fully processed frames are
+delivered to the driver by the firmware. It only has sensor drivers
+for Omnivision OV5647, and Sony IMX219 sensors, and is closed source.
+
+The two drivers are mutually exclusive for the same Unicam instance.
+The firmware checks the device tree configuration during boot. If
+it finds device tree nodes called csi0 or csi1 then it will block the
+firmware from accessing the peripheral, and bcm2835-camera will
+not be able to stream data.
+It should be possible to use bcm2835-camera on one camera interface
+and bcm2835-unicam on the other interface if there is a need to.
+
+Required properties:
+===
+- compatible   : must be "brcm,bcm2835-unicam".
+- reg  : physical base address and length of the register sets for the
+ device.
+- interrupts   : should contain the IRQ line for this Unicam instance.
+- clocks   : list of clock specifiers, corresponding to entries in
+ clock-names property.
+- clock-names  : must contain an "lp_clock" entry, matching entries
+ in the clocks property.
+
+Unicam supports a single port node. It should contain one 'port' child node
+with child 'endpoint' node. Please refer to the bindings defined in
+Documentation/devicetree/bindings/media/video-interfaces.txt.
+
+Within the endpoint node, the following properties are mandatory:
+- remote-endpoint  : links to the source device endpoint.
+- data-lanes   : An array denoting how many data lanes are physically
+ present for this CSI-2 receiver instance. This can
+ be limited by either the SoC itself, or by the
+ breakout on the platform.
+ Lane reordering is not supported, so lanes must be
+ in order, starting at 1.
+
+Lane reordering is not supported on the clock lane, so the optional property
+"clock-lane" will implicitly be <0>.
+Similarly lane inversion is not supported, therefore "lane-polarities" will
+implicitly be <0 0 0 0 0>.
+Neither of these values will be checked.
+
+Example:
+   csi1: csi@7e801000 {
+   compatible = "brcm,bcm2835-unicam";
+   reg = <0x7e801000 0x800>,
+ <0x7e802004 0x4>;
+   interrupts = <2 7>;
+   clocks = < BCM2835_CLOCK_CAM1>;
+   clock-names = "lp_clock";
+
+   port {
+   #address-cells = <1>;
+   #size-cells = <0>;
+
+   csi1_ep: endpoint {
+   remote-endpoint = <_0>;
+   data-lanes = <1 2>;
+   };
+   };
+   };
+
+   i2c0: i2c@7e205000 {
+
+   tc358743: csi-hdmi-bridge@0f {
+   compatible = "toshiba,tc358743";
+   reg = <0x0f>;
+   status = "okay";
+
+   clocks = <_clk>;
+   clock-names = "refclk";
+
+   tc358743_clk: bridge-clk {
+   compatible = "fixed-clock";
+   #clock-cells = <0>;
+   clock-frequency = <2700>;
+   };
+
+   port {
+   tc358743_0: en

[PATCH v2 1/4] [media] v4l2-common: Add helper function for fourcc to string

[Dave Stevenson]

New helper function char *v4l2_fourcc2s(u32 fourcc, char *buf)
that converts a fourcc into a nice printable version.

Signed-off-by: Dave Stevenson <dave.steven...@raspberrypi.org>
---
 drivers/media/v4l2-core/v4l2-common.c | 18 ++
 include/media/v4l2-common.h   |  3 +++
 2 files changed, 21 insertions(+)

diff --git a/drivers/media/v4l2-core/v4l2-common.c 
b/drivers/media/v4l2-core/v4l2-common.c
index a5ea1f5..0219895 100644
--- a/drivers/media/v4l2-core/v4l2-common.c
+++ b/drivers/media/v4l2-core/v4l2-common.c
@@ -405,3 +405,21 @@ void v4l2_get_timestamp(struct timeval *tv)
tv->tv_usec = ts.tv_nsec / NSEC_PER_USEC;
 }
 EXPORT_SYMBOL_GPL(v4l2_get_timestamp);
+
+char *v4l2_fourcc2s(u32 fourcc, char *buf)
+{
+   buf[0] = fourcc & 0x7f;
+   buf[1] = (fourcc >> 8) & 0x7f;
+   buf[2] = (fourcc >> 16) & 0x7f;
+   buf[3] = (fourcc >> 24) & 0x7f;
+   if (fourcc & (1 << 31)) {
+   buf[4] = '-';
+   buf[5] = 'B';
+   buf[6] = 'E';
+   buf[7] = '\0';
+   } else {
+   buf[4] = '\0';
+   }
+   return buf;
+}
+EXPORT_SYMBOL_GPL(v4l2_fourcc2s);
diff --git a/include/media/v4l2-common.h b/include/media/v4l2-common.h
index aac8b7b..5b0fff9 100644
--- a/include/media/v4l2-common.h
+++ b/include/media/v4l2-common.h
@@ -264,4 +264,7 @@ const struct v4l2_frmsize_discrete 
*v4l2_find_nearest_format(
 
 void v4l2_get_timestamp(struct timeval *tv);
 
+#define V4L2_FOURCC_MAX_SIZE 8
+char *v4l2_fourcc2s(u32 fourcc, char *buf);
+
 #endif /* V4L2_COMMON_H_ */
-- 
2.7.4

Re: [RFC 0/2] BCM283x Camera Receiver driver

[Dave Stevenson]

On 30 August 2017 at 11:45, Hans Verkuil <hverk...@xs4all.nl> wrote:
> On 30/08/17 11:40, Dave Stevenson wrote:
>> Hi Hans.
>>
>> On 28 August 2017 at 15:15, Hans Verkuil <hverk...@xs4all.nl> wrote:
>>> Hi Dave,
>>>
>>> What is the status of this work? I ask because I tried to use this driver
>>> plus my tc358743 on my rpi-2b without any luck. Specifically the tc358843
>>> isn't able to read from the i2c bus.
>>
>> I was on other things until last week, but will try to get a V2 sorted
>> either this week or early next.
>> The world moved on slightly too, so there are a few more updates
>> around fwnode stuff that I ought to adopt.
>>
>>> This is probably a bug in my dts, if you have a tree somewhere containing
>>> a working dts for this, then that would be very helpful.
>>
>> Almost certainly just pin ctrl on the I2C bus. The default for i2c0 is
>> normally to GPIOs 0&1 as that is exposed on the 40 pin header
>> (physical pins 27&28). The camera is on GPIOs 28&29 (alt0) for the
>> majority of Pi models (not the Pi3, or the early model B).
>
> Yep, that was the culprit!

Great. I like easy ones :-)

> I now see the tc, but streaming doesn't work yet. I'm not getting any
> interrupts in the unicam driver.

What resolution were you streaming at? There are a couple of things
that come to mind and I don't have solutions for yet.
Firstly the TC358743 driver assumes that all 4 CSI lanes are
available, whilst the Pi only has 2 lanes exposed. IIRC 1080P30 RGB
just trickles over into wanting 3 lanes with the default link
frequencies and you indeed don't get anything if one or more lane is
physically not connected.
Secondly was the FIFOCTL register set via state->pdata.fifo_level that
we discussed before. The default is 16, which is too small for some of
the smaller resolutions. 300 seems reasonable. You do get frames in
that situation, but they're generally corrupt.
I'm intending to try and make the TC358743 more flexible than just
accepting >720P, and if I can support multiple link frequencies then
so much the better as 1080P50 UYVY should be just possible on 2 lane,
however the lower resolutions are unlikely to work due to FIFO
underflow.

> BTW, when s_dv_timings is called, then you need to update the v4l2_format
> as well to the new width and height. I noticed that that didn't happen.

Yes, that came up in the previous discussions and is already fixed.

> Anyway, this is good enough for me for now since I want to add CEC support
> to the tc driver, and I do not need streaming for that...

That sounds fun.
  Dave

Re: [RFC 0/2] BCM283x Camera Receiver driver

[Dave Stevenson]

Hi Hans.

On 28 August 2017 at 15:15, Hans Verkuil <hverk...@xs4all.nl> wrote:
> Hi Dave,
>
> What is the status of this work? I ask because I tried to use this driver
> plus my tc358743 on my rpi-2b without any luck. Specifically the tc358843
> isn't able to read from the i2c bus.

I was on other things until last week, but will try to get a V2 sorted
either this week or early next.
The world moved on slightly too, so there are a few more updates
around fwnode stuff that I ought to adopt.

> This is probably a bug in my dts, if you have a tree somewhere containing
> a working dts for this, then that would be very helpful.

Almost certainly just pin ctrl on the I2C bus. The default for i2c0 is
normally to GPIOs 0&1 as that is exposed on the 40 pin header
(physical pins 27&28). The camera is on GPIOs 28&29 (alt0) for the
majority of Pi models (not the Pi3, or the early model B).

I generally do my config via the DT overlays used in the Pi specific
kernels, but that isn't so directly relevant to mainline.
My 4.9 based WIP project that I'm using on a Pi2 is at
https://github.com/6by9/linux/commits/unicam_wip. Mods to:
- arch/arm/boot/dts/bcm283x.dtsi add the CSI nodes
- arch/arm/boot/dts/overlays/tc358743-overlay.dts adds the relevant
config for TC358743
- /arch/arm/boot/dts/overlays/i2c0-bcm2708-overlay.dts is used with
the pins_28_29 parameter to select 28&29 (that needs updating as it
now uses the bcm2835-i2c driver, not bcm2708-i2c)
Sorry it's piece-meal, but that should give you the bits required.

I hope that helps.
  Dave

> Regards,
>
> Hans
>
> On 14/06/17 17:15, Dave Stevenson wrote:
>> Hi All.
>>
>> This is adding a V4L2 subdevice driver for the CSI2/CCP2 camera
>> receiver peripheral on BCM283x, as used on Raspberry Pi.
>>
>> v4l2-compliance results depend on the sensor subdevice this is
>> connected to. It passes the basic tests cleanly with TC358743,
>> but objects with OV5647
>> fail: v4l2-test-controls.cpp(574): g_ext_ctrls does not support count == 0
>> Neither OV5647 nor Unicam support any controls.
>>
>> I must admit to not having got OV5647 to stream with the current driver
>> register settings. It works with a set of register settings for VGA RAW10.
>> I also have a couple of patches pending for OV5647, but would like to
>> understand the issues better before sending them out.
>>
>> Two queries I do have in V4L2-land:
>> - When s_dv_timings or s_std is called, is the format meant to
>>   be updated automatically? Even if we're already streaming?
>>   Some existing drivers seem to, but others don't.
>> - With s_fmt, is sizeimage settable by the application in the same
>>   way as bytesperline? yavta allows you to specify it on the command
>>   line, whilst v4l2-ctl doesn't. Some of the other parts of the Pi
>>   firmware have a requirement that the buffer is a multiple of 16 lines
>>   high, which can be matched by V4L2 if we can over-allocate the
>>   buffers by the app specifying sizeimage. But if I allow that,
>>   then I get a v4l2-compliance failure as the size doesn't get
>>   reset when switching from RGB3 to UYVY as it takes the request as
>>   a request to over-allocate.
>>
>> Apologies if I've messed up in sending these patches - so many ways
>> to do something.
>>
>> Thanks in advance.
>>   Dave
>>
>> Dave Stevenson (2):
>>   [media] dt-bindings: Document BCM283x CSI2/CCP2 receiver
>>   [media] bcm2835-unicam: Driver for CCP2/CSI2 camera interface
>>
>>  .../devicetree/bindings/media/bcm2835-unicam.txt   |   76 +
>>  drivers/media/platform/Kconfig |1 +
>>  drivers/media/platform/Makefile|2 +
>>  drivers/media/platform/bcm2835/Kconfig |   14 +
>>  drivers/media/platform/bcm2835/Makefile|3 +
>>  drivers/media/platform/bcm2835/bcm2835-unicam.c| 2100 
>> 
>>  drivers/media/platform/bcm2835/vc4-regs-unicam.h   |  257 +++
>>  7 files changed, 2453 insertions(+)
>>  create mode 100644 
>> Documentation/devicetree/bindings/media/bcm2835-unicam.txt
>>  create mode 100644 drivers/media/platform/bcm2835/Kconfig
>>  create mode 100644 drivers/media/platform/bcm2835/Makefile
>>  create mode 100644 drivers/media/platform/bcm2835/bcm2835-unicam.c
>>  create mode 100644 drivers/media/platform/bcm2835/vc4-regs-unicam.h
>>
>

Re: [RFC 2/2] [media] bcm2835-unicam: Driver for CCP2/CSI2 camera interface

[Dave Stevenson]

On 16 June 2017 at 17:08, Hans Verkuil <hverk...@xs4all.nl> wrote:
> On 06/16/2017 05:55 PM, Dave Stevenson wrote:
>>
>> On 16 June 2017 at 15:05, Hans Verkuil <hverk...@xs4all.nl> wrote:
>>>
>>> On 06/15/17 17:11, Dave Stevenson wrote:
>>>>
>>>> On 15 June 2017 at 15:14, Hans Verkuil <hverk...@xs4all.nl> wrote:
>>>>>
>>>>> On 06/15/17 15:38, Dave Stevenson wrote:
>>>>>>
>>>>>> Hi Hans.
>>>>>>
>>>>>> "On 15 June 2017 at 08:12, Hans Verkuil <hverk...@xs4all.nl> wrote:
>>>>>>>
>>>>>>> Hi Dave,
>>>>>>>
>>>>>>> Here is a quick review of this driver. Once a v2 is posted I'll do a
>>>>>>> more
>>>>>>> thorough
>>>>>>> check.
>>>>>>
>>>>>>
>>>>>> Thank you. I wasn't expecting such a quick response.
>>>>>>
>>>>>>> On 06/14/2017 05:15 PM, Dave Stevenson wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> Add driver for the Unicam camera receiver block on
>>>>>>>> BCM283x processors.
>>>>>>>>
>>>>>>>> Signed-off-by: Dave Stevenson <dave.steven...@raspberrypi.org>
>>>>>>>> ---
>>>>>>>>drivers/media/platform/Kconfig   |1 +
>>>>>>>>drivers/media/platform/Makefile  |2 +
>>>>>>>>drivers/media/platform/bcm2835/Kconfig   |   14 +
>>>>>>>>drivers/media/platform/bcm2835/Makefile  |3 +
>>>>>>>>drivers/media/platform/bcm2835/bcm2835-unicam.c  | 2100
>>>>>>>> ++
>>>>>>>>drivers/media/platform/bcm2835/vc4-regs-unicam.h |  257 +++
>>>>>>>>6 files changed, 2377 insertions(+)
>>>>>>>>create mode 100644 drivers/media/platform/bcm2835/Kconfig
>>>>>>>>create mode 100644 drivers/media/platform/bcm2835/Makefile
>>>>>>>>create mode 100644
>>>>>>>> drivers/media/platform/bcm2835/bcm2835-unicam.c
>>>>>>>>create mode 100644
>>>>>>>> drivers/media/platform/bcm2835/vc4-regs-unicam.h
>>>>>>>>
>>>>>>>> +static int unicam_s_input(struct file *file, void *priv, unsigned
>>>>>>>> int i)
>>>>>>>> +{
>>>>>>>> +   struct unicam_device *dev = video_drvdata(file);
>>>>>>>> +   int ret;
>>>>>>>> +
>>>>>>>> +   if (v4l2_subdev_has_op(dev->sensor, video, s_routing))
>>>>>>>> +   ret =  v4l2_subdev_call(dev->sensor, video,
>>>>>>>> s_routing, i,
>>>>>>>> 0, 0);
>>>>>>>> +   else
>>>>>>>> +   ret = -EINVAL;  /* v4l2-compliance insists on
>>>>>>>> -EINVAL */
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> Drop this if-else entirely. s_routing makes really no sense when
>>>>>>> using a
>>>>>>> device
>>>>>>> tree. In this particular case there really is just one input, period.
>>>>>>
>>>>>>
>>>>>> I added this due to the ADV7282-M analogue to CSI bridge chip (uses
>>>>>> adv7180.c driver). It uses s_routing to select the physical input /
>>>>>> input type.
>>>>>> If this is dropped, what is the correct mechanism for selecting the
>>>>>> input? Unless I've missed it, s_routing is not a call that is exposed
>>>>>> to userspace, so we're stuck with composite input 1.
>>>>>>
>>>>>> I had asked this question in previously [1], and whilst Sakari had
>>>>>> kindly replied with "s_routing() video op as it stands now is awful, I
>>>>>> hope no-one uses it", the fact is that it is used.
>>>>>>
>>>>>> [1] http://www.spinics.net/lists/linux-media/msg115550.html
>>>>>
>>>>>
>>>>> s_routing was developed for USB and PCI(e) devices and predat

Re: [RFC 2/2] [media] bcm2835-unicam: Driver for CCP2/CSI2 camera interface

[Dave Stevenson]

On 16 June 2017 at 15:05, Hans Verkuil <hverk...@xs4all.nl> wrote:
> On 06/15/17 17:11, Dave Stevenson wrote:
>> On 15 June 2017 at 15:14, Hans Verkuil <hverk...@xs4all.nl> wrote:
>>> On 06/15/17 15:38, Dave Stevenson wrote:
>>>> Hi Hans.
>>>>
>>>> "On 15 June 2017 at 08:12, Hans Verkuil <hverk...@xs4all.nl> wrote:
>>>>> Hi Dave,
>>>>>
>>>>> Here is a quick review of this driver. Once a v2 is posted I'll do a more
>>>>> thorough
>>>>> check.
>>>>
>>>> Thank you. I wasn't expecting such a quick response.
>>>>
>>>>> On 06/14/2017 05:15 PM, Dave Stevenson wrote:
>>>>>>
>>>>>> Add driver for the Unicam camera receiver block on
>>>>>> BCM283x processors.
>>>>>>
>>>>>> Signed-off-by: Dave Stevenson <dave.steven...@raspberrypi.org>
>>>>>> ---
>>>>>>   drivers/media/platform/Kconfig   |1 +
>>>>>>   drivers/media/platform/Makefile  |2 +
>>>>>>   drivers/media/platform/bcm2835/Kconfig   |   14 +
>>>>>>   drivers/media/platform/bcm2835/Makefile  |3 +
>>>>>>   drivers/media/platform/bcm2835/bcm2835-unicam.c  | 2100
>>>>>> ++
>>>>>>   drivers/media/platform/bcm2835/vc4-regs-unicam.h |  257 +++
>>>>>>   6 files changed, 2377 insertions(+)
>>>>>>   create mode 100644 drivers/media/platform/bcm2835/Kconfig
>>>>>>   create mode 100644 drivers/media/platform/bcm2835/Makefile
>>>>>>   create mode 100644 drivers/media/platform/bcm2835/bcm2835-unicam.c
>>>>>>   create mode 100644 drivers/media/platform/bcm2835/vc4-regs-unicam.h
>>>>>>
>>>>>> +static int unicam_s_input(struct file *file, void *priv, unsigned int i)
>>>>>> +{
>>>>>> +   struct unicam_device *dev = video_drvdata(file);
>>>>>> +   int ret;
>>>>>> +
>>>>>> +   if (v4l2_subdev_has_op(dev->sensor, video, s_routing))
>>>>>> +   ret =  v4l2_subdev_call(dev->sensor, video, s_routing, i,
>>>>>> 0, 0);
>>>>>> +   else
>>>>>> +   ret = -EINVAL;  /* v4l2-compliance insists on -EINVAL */
>>>>>
>>>>>
>>>>> Drop this if-else entirely. s_routing makes really no sense when using a
>>>>> device
>>>>> tree. In this particular case there really is just one input, period.
>>>>
>>>> I added this due to the ADV7282-M analogue to CSI bridge chip (uses
>>>> adv7180.c driver). It uses s_routing to select the physical input /
>>>> input type.
>>>> If this is dropped, what is the correct mechanism for selecting the
>>>> input? Unless I've missed it, s_routing is not a call that is exposed
>>>> to userspace, so we're stuck with composite input 1.
>>>>
>>>> I had asked this question in previously [1], and whilst Sakari had
>>>> kindly replied with "s_routing() video op as it stands now is awful, I
>>>> hope no-one uses it", the fact is that it is used.
>>>>
>>>> [1] http://www.spinics.net/lists/linux-media/msg115550.html
>>>
>>> s_routing was developed for USB and PCI(e) devices and predates the device 
>>> tree.
>>> Basically USB and PCI drivers will have card definitions where USB/PCI card 
>>> IDs
>>> are mapped to card descriptions, and that includes information on the 
>>> various
>>> inputs (composite, S-Video, etc) that are available on the backplane and 
>>> how those
>>> physical connectors are hooked up to the pins on the video ICs.
>>>
>>> The enum/s/g_input ioctls all show the end-user view, i.e. they enumerate 
>>> the
>>> inputs on the backpanel of the product. The s_routing op was created to map
>>> such inputs to actual pins on the ICs.
>>>
>>> For platform devices we would do this in the device tree today, but some of
>>> the necessary bindings are still missing. Specifically those for connectors,
>>> AFAIK those are not yet defined. It's been discussed, but never finalized.
>>>
>>> So if this was done correctly you would use the connector endpoints in the
>>> device tree to enumerate the inputs a

Re: [RFC 1/2] [media] dt-bindings: Document BCM283x CSI2/CCP2 receiver

[Dave Stevenson]

Hi Sakari

On 16 June 2017 at 10:57, Sakari Ailus <sakari.ai...@iki.fi> wrote:
> Hi Dave,
>
> On Thu, Jun 15, 2017 at 05:15:04PM +0100, Dave Stevenson wrote:
>> Hi Sakari.
>>
>> Thanks for the review.
>>
>> On 15 June 2017 at 13:59, Sakari Ailus <sakari.ai...@iki.fi> wrote:
>> > Hi Dave,
>> >
>> > Thanks for the set!
>> >
>> > On Wed, Jun 14, 2017 at 04:15:46PM +0100, Dave Stevenson wrote:
>> >> Document the DT bindings for the CSI2/CCP2 receiver peripheral
>> >> (known as Unicam) on BCM283x SoCs.
>> >>
>> >> Signed-off-by: Dave Stevenson <dave.steven...@raspberrypi.org>
>> >> ---
>> >>  .../devicetree/bindings/media/bcm2835-unicam.txt   | 76 
>> >> ++
>> >>  1 file changed, 76 insertions(+)
>> >>  create mode 100644 
>> >> Documentation/devicetree/bindings/media/bcm2835-unicam.txt
>> >>
>> >> diff --git a/Documentation/devicetree/bindings/media/bcm2835-unicam.txt 
>> >> b/Documentation/devicetree/bindings/media/bcm2835-unicam.txt
>> >> new file mode 100644
>> >> index 000..cc5a451
>> >> --- /dev/null
>> >> +++ b/Documentation/devicetree/bindings/media/bcm2835-unicam.txt
>> >> @@ -0,0 +1,76 @@
>> >> +Broadcom BCM283x Camera Interface (Unicam)
>> >> +--
>> >> +
>> >> +The Unicam block on BCM283x SoCs is the receiver for either
>> >> +CSI-2 or CCP2 data from image sensors or similar devices.
>> >> +
>> >> +Required properties:
>> >> +===
>> >> +- compatible : must be "brcm,bcm2835-unicam".
>> >> +- reg: physical base address and length of the register 
>> >> sets for the
>> >> +   device.
>> >> +- interrupts : should contain the IRQ line for this Unicam instance.
>> >> +- clocks : list of clock specifiers, corresponding to entries in
>> >> +   clock-names property.
>> >> +- clock-names: must contain an "lp_clock" entry, matching entries
>> >> +   in the clocks property.
>> >> +
>> >> +Optional properties
>> >> +===
>> >> +- max-data-lanes: the hardware can support varying numbers of clock 
>> >> lanes.
>> >> +   This value is the maximum number supported by this 
>> >> instance.
>> >> +   Known values of 2 or 4. Default is 2.
>> >
>> > Please use "data-lanes" endpoint property instead. This is the number of
>> > connected physical lanes and specific to the hardware.
>>
>> I'll rethink/test, but to explain what I was intending to achieve:
>>
>> Registers UNICAM_DAT2 and UNICAM_DAT3 are not valid for instances of
>> the hardware that only have two lanes instantiated in silicon.
>> In the case of the whole BCM283x family, Unicam0 ony has 2 lanes
>> instantiated, whilst Unicam1 has the maximum 4 lanes. (Lower
>> resolution front cameras would connect to Unicam0, whilst the higher
>> resolution back cameras would go to Unicam1).
>>
>> To further confuse matters, on the Pi platforms (other than the
>> Compute Module), it is Unicam1 that is brought out to the camera
>> connector but with only 2 lanes wired up.
>
> This information should be present in the DT. I.e. the data-lanes property.
>
> v4l2_fwnode_endpoint_alloc_parse() can obtain that from DT, among other
> things (I haven't checked the second patch yet).

I was interpreting data-lanes as being a function of the CSI-2 source
only, not the CSI-2 sink.
I haven't seen any examples where it has been it has been set on a
sink, but if that is valid then it sounds like a sensible thing to do.
...
OK, I'd missed it in video_interfaces.txt where it has been set for
the sh-mobile-csi2 endpoint (not that there appears to be a driver
advertising sh-mobile-csi2 as a compatible string anymore - it was
removed in 4.9).

It sounds like adopting a sink endpoint property of "data-lanes"  is
reasonable. Make it optional with the driver adopting a default "<1
2>" and you'll cover 99% of the cases.
On Compute Modules it can be overridden to "<1 2 3 4>" for Unicam1.
Selecting more lanes on Unicam0 is just an error that the DT author
has to sort out.

>>
>> I was intending to make it possible for the driver to avoid writing to
>> invalid registers, and also describe the platform 

Re: [RFC 1/2] [media] dt-bindings: Document BCM283x CSI2/CCP2 receiver

[Dave Stevenson]

Hi Sakari.

Thanks for the review.

On 15 June 2017 at 13:59, Sakari Ailus <sakari.ai...@iki.fi> wrote:
> Hi Dave,
>
> Thanks for the set!
>
> On Wed, Jun 14, 2017 at 04:15:46PM +0100, Dave Stevenson wrote:
>> Document the DT bindings for the CSI2/CCP2 receiver peripheral
>> (known as Unicam) on BCM283x SoCs.
>>
>> Signed-off-by: Dave Stevenson <dave.steven...@raspberrypi.org>
>> ---
>>  .../devicetree/bindings/media/bcm2835-unicam.txt   | 76 
>> ++
>>  1 file changed, 76 insertions(+)
>>  create mode 100644 
>> Documentation/devicetree/bindings/media/bcm2835-unicam.txt
>>
>> diff --git a/Documentation/devicetree/bindings/media/bcm2835-unicam.txt 
>> b/Documentation/devicetree/bindings/media/bcm2835-unicam.txt
>> new file mode 100644
>> index 000..cc5a451
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/media/bcm2835-unicam.txt
>> @@ -0,0 +1,76 @@
>> +Broadcom BCM283x Camera Interface (Unicam)
>> +--
>> +
>> +The Unicam block on BCM283x SoCs is the receiver for either
>> +CSI-2 or CCP2 data from image sensors or similar devices.
>> +
>> +Required properties:
>> +===
>> +- compatible : must be "brcm,bcm2835-unicam".
>> +- reg: physical base address and length of the register 
>> sets for the
>> +   device.
>> +- interrupts : should contain the IRQ line for this Unicam instance.
>> +- clocks : list of clock specifiers, corresponding to entries in
>> +   clock-names property.
>> +- clock-names: must contain an "lp_clock" entry, matching entries
>> +   in the clocks property.
>> +
>> +Optional properties
>> +===
>> +- max-data-lanes: the hardware can support varying numbers of clock lanes.
>> +   This value is the maximum number supported by this instance.
>> +   Known values of 2 or 4. Default is 2.
>
> Please use "data-lanes" endpoint property instead. This is the number of
> connected physical lanes and specific to the hardware.

I'll rethink/test, but to explain what I was intending to achieve:

Registers UNICAM_DAT2 and UNICAM_DAT3 are not valid for instances of
the hardware that only have two lanes instantiated in silicon.
In the case of the whole BCM283x family, Unicam0 ony has 2 lanes
instantiated, whilst Unicam1 has the maximum 4 lanes. (Lower
resolution front cameras would connect to Unicam0, whilst the higher
resolution back cameras would go to Unicam1).

To further confuse matters, on the Pi platforms (other than the
Compute Module), it is Unicam1 that is brought out to the camera
connector but with only 2 lanes wired up.

I was intending to make it possible for the driver to avoid writing to
invalid registers, and also describe the platform limitations to allow
sanity checking.
I haven't tested against Unicam0 as yet to see what actually happens
if we try to write UNICAM_DAT2 or UNICAM_DAT3. If the hardware
silently swallows it then that requirement is null and void. I'll do
some testing tomorrow.
The second bit comes down to how friendly an error you want should
someone write an invalid DT with 'data-lanes' greater than can be
supported by the platform.

> Could you also document which endpoint properties are mandatory and which
> ones optional?

Will do, although I'm not sure there are any required properties.

>> +
>> +
>> +Unicam supports a single port node. It should contain one 'port' child node
>> +with child 'endpoint' node. Please refer to the bindings defined in
>> +Documentation/devicetree/bindings/media/video-interfaces.txt.
>> +
>> +Example:
>> + csi1: csi@7e801000 {
>> + compatible = "brcm,bcm2835-unicam";
>> + reg = <0x7e801000 0x800>,
>> +   <0x7e802004 0x4>;
>> + interrupts = <2 7>;
>> + clocks = < BCM2835_CLOCK_CAM1>;
>> + clock-names = "lp_clock";
>> +
>> + port {
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> +
>> + endpoint {
>> + remote-endpoint = <_0>;
>> +
>
> Extra newline. Don't you need any other properties here?

Newline done.
As above, I don't believe there are any other properties required, but
will double check. What extras were you expecting to see there?

>> + };
>> + };
>> + };
>> +

Re: [RFC 2/2] [media] bcm2835-unicam: Driver for CCP2/CSI2 camera interface

[Dave Stevenson]

Hi Stefan.

On 15 June 2017 at 15:49, Stefan Wahren <stefan.wah...@i2se.com> wrote:
> Hi Dave,
>
> Am 15.06.2017 um 15:38 schrieb Dave Stevenson:
>> Hi Hans.
>>
>> "On 15 June 2017 at 08:12, Hans Verkuil <hverk...@xs4all.nl> wrote:
>>> Hi Dave,
>>>
>>> Here is a quick review of this driver. Once a v2 is posted I'll do a more
>>> thorough
>>> check.
>> Thank you. I wasn't expecting such a quick response.
>>
>>> On 06/14/2017 05:15 PM, Dave Stevenson wrote:
>>>> ...
>>>>
>>>> +
>>>> +struct bayer_fmt {
>>>> +   u32 fourcc;
>>>> +   u8 depth;
>>>> +};
>>>> +
>>>> +const struct bayer_fmt all_bayer_bggr[] = {
>>>> +   {V4L2_PIX_FMT_SBGGR8,   8},
>>>> +   {V4L2_PIX_FMT_SBGGR10P, 10},
>>>> +   {V4L2_PIX_FMT_SBGGR12,  12},
>>>> +   {V4L2_PIX_FMT_SBGGR16,  16},
>>>> +   {0, 0}
>>>> +};
>>>> +
>>>> +const struct bayer_fmt all_bayer_rggb[] = {
>>>> +   {V4L2_PIX_FMT_SRGGB8,   8},
>>>> +   {V4L2_PIX_FMT_SRGGB10P, 10},
>>>> +   {V4L2_PIX_FMT_SRGGB12,  12},
>>>> +   /* V4L2_PIX_FMT_SRGGB16,16},*/
>>>
>>> Why is this commented out? Either uncomment, add a proper comment explaining
>>> why
>>> or remove it.
>> I was developing this against the Pi specific tree, and that is still
>> on 4.9 which didn't have several of the 16 bit Bayer formats. I see
>> that Sakari has added them (thank you Sakari), so I can uncomment
>> them.
>
> does this series work with Linux Mainline (incl. bcm283x dts files)?
>
> In case not, please tell what is missing?

I switched about a week or so back onto mainline, partly to pick up
and use the recent fwnode parsing changes within V4L2.
This driver is working with Mainline and bcm283x.dts once the relevant
DT sections are added.

The DT changes aren't in a state to post as a patch set as yet. The
main stumbiling block is that the camera I2C is BSC0 but typically on
GPIOs 28&29 instead of 0&1 (44&45 on Pi3). Swapping to 28&29 would be
a significant change in behaviour so wouldn't be acceptable.
If it can be knocked in to shape then the i2c-mux-pinctrl driver
appears to do what is required to make 0&1 and 28&29 appear as 2
independent I2C buses, but I haven't had the time as yet to finesse
that. It needs some care as 44&45 (needed for the camera on Pi3) are
used for the SMSC9514 clock and audio on Pi2, so the configuration
needs to be sorted per platform. (switching off the 9514 clock results
in no ethernet or USB, so generally means time for a swift reboot -
been there, done that).

The DT diffs I have for running on a Pi2 are:
diff --git a/arch/arm/boot/dts/bcm2835-rpi.dtsi
b/arch/arm/boot/dts/bcm2835-rpi.dtsi
index a7b5ce1..1f24219 100644
--- a/arch/arm/boot/dts/bcm2835-rpi.dtsi
+++ b/arch/arm/boot/dts/bcm2835-rpi.dtsi
@@ -46,7 +46,7 @@

  {
pinctrl-names = "default";
-   pinctrl-0 = <_gpio0>;
+   pinctrl-0 = <_gpio28>;
status = "okay";
clock-frequency = <10>;
 };
@@ -106,3 +106,11 @@
  {
power-domains = < RPI_POWER_DOMAIN_DSI1>;
 };
+
+ {
+   power-domains = < RPI_POWER_DOMAIN_UNICAM0>;
+};
+
+ {
+   power-domains = < RPI_POWER_DOMAIN_UNICAM1>;
+};
diff --git a/arch/arm/boot/dts/bcm283x.dtsi b/arch/arm/boot/dts/bcm283x.dtsi
index 561f27d..4c575e4 100644
--- a/arch/arm/boot/dts/bcm283x.dtsi
+++ b/arch/arm/boot/dts/bcm283x.dtsi
@@ -512,6 +512,34 @@
status = "disabled";
};

+   csi0: csi0@7e80 {
+   compatible = "brcm,bcm2835-unicam";
+   reg = <0x7e80 0x800>,
+ <0x7e802000 0x4>;
+   interrupts = <2 6>;
+   clocks = < BCM2835_CLOCK_CAM0>;
+   clock-names = "lp_clock";
+   #address-cells = <1>;
+   #size-cells = <0>;
+   #clock-cells = <1>;
+
+   status = "disabled";
+   };
+
+   csi1: csi1@7e801000 {
+   compatible = "brcm,bcm2835-unicam";
+   reg = <0x7e801000 0x800>,
+ <0x7e802004 0x4>;
+   interrupts = <2 7>;
+   clocks = < BCM2835_CLOCK_CAM1>;
+   clock-names = "lp_clock";
+   #address-cells = <1>;
+

Re: [RFC 2/2] [media] bcm2835-unicam: Driver for CCP2/CSI2 camera interface

[Dave Stevenson]

On 15 June 2017 at 15:14, Hans Verkuil <hverk...@xs4all.nl> wrote:
> On 06/15/17 15:38, Dave Stevenson wrote:
>> Hi Hans.
>>
>> "On 15 June 2017 at 08:12, Hans Verkuil <hverk...@xs4all.nl> wrote:
>>> Hi Dave,
>>>
>>> Here is a quick review of this driver. Once a v2 is posted I'll do a more
>>> thorough
>>> check.
>>
>> Thank you. I wasn't expecting such a quick response.
>>
>>> On 06/14/2017 05:15 PM, Dave Stevenson wrote:
>>>>
>>>> Add driver for the Unicam camera receiver block on
>>>> BCM283x processors.
>>>>
>>>> Signed-off-by: Dave Stevenson <dave.steven...@raspberrypi.org>
>>>> ---
>>>>   drivers/media/platform/Kconfig   |1 +
>>>>   drivers/media/platform/Makefile  |2 +
>>>>   drivers/media/platform/bcm2835/Kconfig   |   14 +
>>>>   drivers/media/platform/bcm2835/Makefile  |3 +
>>>>   drivers/media/platform/bcm2835/bcm2835-unicam.c  | 2100
>>>> ++
>>>>   drivers/media/platform/bcm2835/vc4-regs-unicam.h |  257 +++
>>>>   6 files changed, 2377 insertions(+)
>>>>   create mode 100644 drivers/media/platform/bcm2835/Kconfig
>>>>   create mode 100644 drivers/media/platform/bcm2835/Makefile
>>>>   create mode 100644 drivers/media/platform/bcm2835/bcm2835-unicam.c
>>>>   create mode 100644 drivers/media/platform/bcm2835/vc4-regs-unicam.h
>>>>
>>>> +static int unicam_s_input(struct file *file, void *priv, unsigned int i)
>>>> +{
>>>> +   struct unicam_device *dev = video_drvdata(file);
>>>> +   int ret;
>>>> +
>>>> +   if (v4l2_subdev_has_op(dev->sensor, video, s_routing))
>>>> +   ret =  v4l2_subdev_call(dev->sensor, video, s_routing, i,
>>>> 0, 0);
>>>> +   else
>>>> +   ret = -EINVAL;  /* v4l2-compliance insists on -EINVAL */
>>>
>>>
>>> Drop this if-else entirely. s_routing makes really no sense when using a
>>> device
>>> tree. In this particular case there really is just one input, period.
>>
>> I added this due to the ADV7282-M analogue to CSI bridge chip (uses
>> adv7180.c driver). It uses s_routing to select the physical input /
>> input type.
>> If this is dropped, what is the correct mechanism for selecting the
>> input? Unless I've missed it, s_routing is not a call that is exposed
>> to userspace, so we're stuck with composite input 1.
>>
>> I had asked this question in previously [1], and whilst Sakari had
>> kindly replied with "s_routing() video op as it stands now is awful, I
>> hope no-one uses it", the fact is that it is used.
>>
>> [1] http://www.spinics.net/lists/linux-media/msg115550.html
>
> s_routing was developed for USB and PCI(e) devices and predates the device 
> tree.
> Basically USB and PCI drivers will have card definitions where USB/PCI card 
> IDs
> are mapped to card descriptions, and that includes information on the various
> inputs (composite, S-Video, etc) that are available on the backplane and how 
> those
> physical connectors are hooked up to the pins on the video ICs.
>
> The enum/s/g_input ioctls all show the end-user view, i.e. they enumerate the
> inputs on the backpanel of the product. The s_routing op was created to map
> such inputs to actual pins on the ICs.
>
> For platform devices we would do this in the device tree today, but some of
> the necessary bindings are still missing. Specifically those for connectors,
> AFAIK those are not yet defined. It's been discussed, but never finalized.
>
> So if this was done correctly you would use the connector endpoints in the
> device tree to enumerate the inputs and use how they are connected to the
> other blocks as the routing information (i.e. pad number).
>
> I would say that is the advanced course and to do this later.

Certainly the advanced course, but I'm still not seeing how that all
hangs together.

To me that all sounds like stuff that ought to be within the ADV
driver? From my perspective as the CSI-2 receiver I only have one
input.
So how does the application select between those inputs?

Having had a bit of a grep I think the tvp5150 driver is doing what
you're suggesting. However that appears to force you into using the
media controller API. Is that not overkill particularly from an
application perspective?

You also say above that enum/s/g_input is all about switching between
physical connectors, and that's what I'm doi

Re: [RFC 2/2] [media] bcm2835-unicam: Driver for CCP2/CSI2 camera interface

[Dave Stevenson]

Hi Hans.

"On 15 June 2017 at 08:12, Hans Verkuil <hverk...@xs4all.nl> wrote:
> Hi Dave,
>
> Here is a quick review of this driver. Once a v2 is posted I'll do a more
> thorough
> check.

Thank you. I wasn't expecting such a quick response.

> On 06/14/2017 05:15 PM, Dave Stevenson wrote:
>>
>> Add driver for the Unicam camera receiver block on
>> BCM283x processors.
>>
>> Signed-off-by: Dave Stevenson <dave.steven...@raspberrypi.org>
>> ---
>>   drivers/media/platform/Kconfig   |1 +
>>   drivers/media/platform/Makefile  |2 +
>>   drivers/media/platform/bcm2835/Kconfig   |   14 +
>>   drivers/media/platform/bcm2835/Makefile  |3 +
>>   drivers/media/platform/bcm2835/bcm2835-unicam.c  | 2100
>> ++
>>   drivers/media/platform/bcm2835/vc4-regs-unicam.h |  257 +++
>>   6 files changed, 2377 insertions(+)
>>   create mode 100644 drivers/media/platform/bcm2835/Kconfig
>>   create mode 100644 drivers/media/platform/bcm2835/Makefile
>>   create mode 100644 drivers/media/platform/bcm2835/bcm2835-unicam.c
>>   create mode 100644 drivers/media/platform/bcm2835/vc4-regs-unicam.h
>>
>> diff --git a/drivers/media/platform/Kconfig
>> b/drivers/media/platform/Kconfig
>> index 8da521a..aa9 100644
>> --- a/drivers/media/platform/Kconfig
>> +++ b/drivers/media/platform/Kconfig
>> @@ -135,6 +135,7 @@ source "drivers/media/platform/am437x/Kconfig"
>>   source "drivers/media/platform/xilinx/Kconfig"
>>   source "drivers/media/platform/rcar-vin/Kconfig"
>>   source "drivers/media/platform/atmel/Kconfig"
>> +source "drivers/media/platform/bcm2835/Kconfig"
>> config VIDEO_TI_CAL
>> tristate "TI CAL (Camera Adaptation Layer) driver"
>> diff --git a/drivers/media/platform/Makefile
>> b/drivers/media/platform/Makefile
>> index 6bbdf94..9c5e412 100644
>> --- a/drivers/media/platform/Makefile
>> +++ b/drivers/media/platform/Makefile
>> @@ -81,3 +81,5 @@ obj-$(CONFIG_VIDEO_MEDIATEK_VCODEC)   += mtk-vcodec/
>>   obj-$(CONFIG_VIDEO_MEDIATEK_MDP)  += mtk-mdp/
>> obj-$(CONFIG_VIDEO_MEDIATEK_JPEG)   += mtk-jpeg/
>> +
>> +obj-y  += bcm2835/
>> diff --git a/drivers/media/platform/bcm2835/Kconfig
>> b/drivers/media/platform/bcm2835/Kconfig
>> new file mode 100644
>> index 000..9f9be9e
>> --- /dev/null
>> +++ b/drivers/media/platform/bcm2835/Kconfig
>> @@ -0,0 +1,14 @@
>> +# Broadcom VideoCore4 V4L2 camera support
>> +
>> +config VIDEO_BCM2835_UNICAM
>> +   tristate "Broadcom BCM2835 Unicam video capture driver"
>> +   depends on VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API
>> +   depends on ARCH_BCM2708 || ARCH_BCM2709 || ARCH_BCM2835 ||
>> COMPILE_TEST
>
>
> So this block is available on other broadcom SoCs as well? Not just the
> 2835?
> Should the description of this Kconfig be adapted?

As Stefan has noted, BCM2708 and BCM2709 are only in the downstream
tree and will be removed.

Techincally this block is also present in a few other Broadcom SoCs
that use VideoCore4 (eg BCM28155), however 283x is the only chip
family that I can support.
There are Broadcom folk supporting 281xx, so I will cc them when I
come to adding an entry in MAINTAINERS (I've already communicated with
Eric, and he was happy to defer this driver off to me. Exact details
to be agreed).

>> +   select VIDEOBUF2_DMA_CONTIG
>> +   select V4L2_FWNODE
>> +   ---help---
>> + Say Y here to enable V4L2 subdevice for CSI2 receiver.
>> + This is a V4L2 subdevice that interfaces directly to the VC4
>> peripheral.
>> +
>> +  To compile this driver as a module, choose M here. The module
>> +  will be called bcm2835-unicam.
>> diff --git a/drivers/media/platform/bcm2835/Makefile
>> b/drivers/media/platform/bcm2835/Makefile
>> new file mode 100644
>> index 000..a98aba0
>> --- /dev/null
>> +++ b/drivers/media/platform/bcm2835/Makefile
>> @@ -0,0 +1,3 @@
>> +# Makefile for BCM2835 Unicam driver
>> +
>> +obj-$(CONFIG_VIDEO_BCM2835_UNICAM) += bcm2835-unicam.o
>> diff --git a/drivers/media/platform/bcm2835/bcm2835-unicam.c
>> b/drivers/media/platform/bcm2835/bcm2835-unicam.c
>> new file mode 100644
>> index 000..26039da
>> --- /dev/null
>> +++ b/drivers/media/platform/bcm2835/bcm2835-unicam.c
>> @@ -0,0 +1,2100 @@
>> +/*
>> + * BCM2835 Unicam capture Driver
>&

Re: [RFC 1/2] [media] dt-bindings: Document BCM283x CSI2/CCP2 receiver

[Dave Stevenson]

Hi Stefan.
Thanks for taking the time to review this.

On 15 June 2017 at 07:34, Stefan Wahren <stefan.wah...@i2se.com> wrote:
> Hi Dave,
>
> Am 14.06.2017 um 17:15 schrieb Dave Stevenson:
>> Document the DT bindings for the CSI2/CCP2 receiver peripheral
>> (known as Unicam) on BCM283x SoCs.
>>
>> Signed-off-by: Dave Stevenson <dave.steven...@raspberrypi.org>
>
> please add the devicetree guys in CC for the binding.

Will do for V2.

>> ---
>>  .../devicetree/bindings/media/bcm2835-unicam.txt   | 76 
>> ++
>>  1 file changed, 76 insertions(+)
>>  create mode 100644 
>> Documentation/devicetree/bindings/media/bcm2835-unicam.txt
>>
>> diff --git a/Documentation/devicetree/bindings/media/bcm2835-unicam.txt 
>> b/Documentation/devicetree/bindings/media/bcm2835-unicam.txt
>> new file mode 100644
>> index 000..cc5a451
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/media/bcm2835-unicam.txt
>> @@ -0,0 +1,76 @@
>> +Broadcom BCM283x Camera Interface (Unicam)
>> +--
>> +
>> +The Unicam block on BCM283x SoCs is the receiver for either
>> +CSI-2 or CCP2 data from image sensors or similar devices.
>
> It would be nice to add some of your explanations to Hans in this
> document or into the driver.

Will do.

>> +
>> +Required properties:
>> +===
>> +- compatible : must be "brcm,bcm2835-unicam".
>> +- reg: physical base address and length of the register 
>> sets for the
>> +   device.
>> +- interrupts : should contain the IRQ line for this Unicam instance.
>> +- clocks : list of clock specifiers, corresponding to entries in
>> +   clock-names property.
>> +- clock-names: must contain an "lp_clock" entry, matching entries
>> +   in the clocks property.
>> +
>> +Optional properties
>> +===
>> +- max-data-lanes: the hardware can support varying numbers of clock lanes.
>> +   This value is the maximum number supported by this instance.
>> +   Known values of 2 or 4. Default is 2.
>
> AFAIK, this isn't a common property yet. So possibly a vendor prefix
> must be added.

I think yoiu are correct that it isn't a common property. I was
wanting to cover the situation on the majority of the Pi boards where
Unicam1 has been used which can handle 4 lanes, but only 2 have been
broken out to the camera connector.
Happy to add a vendor prefix in V2.

>> +
>> +
>> +Unicam supports a single port node. It should contain one 'port' child node
>> +with child 'endpoint' node. Please refer to the bindings defined in
>> +Documentation/devicetree/bindings/media/video-interfaces.txt.
>> +
>> +Example:
>> + csi1: csi@7e801000 {
>> + compatible = "brcm,bcm2835-unicam";
>> + reg = <0x7e801000 0x800>,
>> +   <0x7e802004 0x4>;
>> + interrupts = <2 7>;
>> + clocks = < BCM2835_CLOCK_CAM1>;
>> + clock-names = "lp_clock";
>> +
>> + port {
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> +
>> + endpoint {
>> + remote-endpoint = <_0>;
>> +
>> + };
>> + };
>> + };
>> +
>> + i2c0: i2c@7e205000 {
>> +
>> + tc358743: tc358743@0f {
>
> Usually the node name should describe the function of the node for example:
>
> tc358743: csi-hdmi-bridge@0f

Will do.

> Best regards
> Stefan
>
>> + compatible = "toshiba,tc358743";
>> + reg = <0x0f>;
>> + status = "okay";
>> +
>> + clocks = <_clk>;
>> + clock-names = "refclk";
>> +
>> + tc358743_clk: bridge-clk {
>> + compatible = "fixed-clock";
>> + #clock-cells = <0>;
>> + clock-frequency = <2700>;
>> + };
>> +
>> + port {
>> + tc358743_0: endpoint {
>> + remote-endpoint = <>;
>> + clock-lanes = <0>;
>> + data-lanes = <1 2 3 4>;
>> + clock-noncontinuous;
>> + link-frequencies =
>> + /bits/ 64 <29700>;
>> + };
>> + };
>> + };
>> + };

Thanks
  Dave

Re: [RFC 0/2] BCM283x Camera Receiver driver

[Dave Stevenson]

On 15 June 2017 at 08:17, Hans Verkuil <hverk...@xs4all.nl> wrote:
> On 06/14/2017 11:03 PM, Dave Stevenson wrote:
>>
>> On 14 June 2017 at 18:38, Hans Verkuil <hverk...@xs4all.nl> wrote:
>>>
>>> On 06/14/2017 06:29 PM, Dave Stevenson wrote:
>>>>
>>>>
>>>> Hi Hans.
>>>>
>>>> On 14 June 2017 at 16:42, Hans Verkuil <hverk...@xs4all.nl> wrote:
>>>>>
>>>>>
>>>>> Hi Dave,
>>>>>
>>>>> How does this driver relate to this staging driver:
>>>>>
>>>>> drivers/staging/vc04_services/bcm2835-camera/
>>>>>
>>>>> It's not obvious to me.
>>>>
>>>>
>>>>
>>>> drivers/staging/vc04_services/bcm2835-camera/ is using the VideoCore
>>>> firmware to control Unicam, ISP, and all the tuner algorithms. The ARM
>>>> gets delivered fully processed buffers from the VideoCore side. The
>>>> firmware only has drivers for the Omnivision OV5647 and Sony IMX219
>>>> (and an unsupported one for the Toshiba TC358743).
>>>>
>>>> This driver is solely the Unicam block, reading the data in over
>>>> CSI2/CCP2 from the sensor and writing it to memory. No ISP or control
>>>> loops.
>>>> Other than power management, this driver is running solely on the ARM
>>>> with no involvement from the VideoCore firmware.
>>>> The sensor driver is whatever suitable V4L2 subdevice driver you fancy
>>>> attaching (as long as it supports CSI2, or eventually CCP2).
>>>
>>>
>>>
>>> What is the interaction between these two drivers? Can they co-exist?
>>> I would expect them to be mutually exclusive.
>>
>>
>> Mutually exclusive for the same Unicam instance, yes.
>>
>> There are two Unicam instances on all BCM283x chips and both are
>> brought out on the Compute Modules. You could run bcm2835-unicam on
>> one and bcm2835-camera on the other if you so wished.
>> The firmware checks whether the csi nodes in the device tree are
>> active, and won't touch them if they are.
>
>
> It would be good if this explanation is mentioned both in the driver code
> and (I think) in the bindings document of *both* drivers. This setup is
> unusual, so some extra documentation isn't amiss.

OK, will add it to this driver for V2.
The other driver cleanups are on my to-do list. Would you object
horrendously if I deferred adding the explanation to that driver to
when I'm doing those cleanups?

> Regards,
>
> Hans

Re: [RFC 0/2] BCM283x Camera Receiver driver

[Dave Stevenson]

On 14 June 2017 at 18:38, Hans Verkuil <hverk...@xs4all.nl> wrote:
> On 06/14/2017 06:29 PM, Dave Stevenson wrote:
>>
>> Hi Hans.
>>
>> On 14 June 2017 at 16:42, Hans Verkuil <hverk...@xs4all.nl> wrote:
>>>
>>> Hi Dave,
>>>
>>> How does this driver relate to this staging driver:
>>>
>>> drivers/staging/vc04_services/bcm2835-camera/
>>>
>>> It's not obvious to me.
>>
>>
>> drivers/staging/vc04_services/bcm2835-camera/ is using the VideoCore
>> firmware to control Unicam, ISP, and all the tuner algorithms. The ARM
>> gets delivered fully processed buffers from the VideoCore side. The
>> firmware only has drivers for the Omnivision OV5647 and Sony IMX219
>> (and an unsupported one for the Toshiba TC358743).
>>
>> This driver is solely the Unicam block, reading the data in over
>> CSI2/CCP2 from the sensor and writing it to memory. No ISP or control
>> loops.
>> Other than power management, this driver is running solely on the ARM
>> with no involvement from the VideoCore firmware.
>> The sensor driver is whatever suitable V4L2 subdevice driver you fancy
>> attaching (as long as it supports CSI2, or eventually CCP2).
>
>
> What is the interaction between these two drivers? Can they co-exist?
> I would expect them to be mutually exclusive.

Mutually exclusive for the same Unicam instance, yes.

There are two Unicam instances on all BCM283x chips and both are
brought out on the Compute Modules. You could run bcm2835-unicam on
one and bcm2835-camera on the other if you so wished.
The firmware checks whether the csi nodes in the device tree are
active, and won't touch them if they are.

>>
>>> On 06/14/2017 05:15 PM, Dave Stevenson wrote:
>>>>
>>>>
>>>> Hi All.
>>>>
>>>> This is adding a V4L2 subdevice driver for the CSI2/CCP2 camera
>>>> receiver peripheral on BCM283x, as used on Raspberry Pi.
>>>>
>>>> v4l2-compliance results depend on the sensor subdevice this is
>>>> connected to. It passes the basic tests cleanly with TC358743,
>>>> but objects with OV5647
>>>> fail: v4l2-test-controls.cpp(574): g_ext_ctrls does not support count ==
>>>> 0
>>>> Neither OV5647 nor Unicam support any controls.
>>>
>>>
>>>
>>> Are you compiling v4l2-compliance from the v4l-utils git repo? If not,
>>> then please do so and run again. The version packaged by distros tends
>>> to be seriously outdated.
>>
>>
>> Yes, I'm building from v4l-utils.git.
>> I updated within the last week, although you appear to have added 2
>> commits since (both CEC related).
>> I'm on "ef074cf media-ctl: add colorimetry support"
>
>
> But the line with that error is at line number 587 in my repo, not 574.
> So I'm a bit suspicious.
>
> Anyway, can you give the output of 'v4l2-ctl -l'?

Will do tomorrow. I'll update to the latest v4l2-conformance too to
avoid confusion.

>>
>>>> I must admit to not having got OV5647 to stream with the current driver
>>>> register settings. It works with a set of register settings for VGA
>>>> RAW10.
>>>> I also have a couple of patches pending for OV5647, but would like to
>>>> understand the issues better before sending them out.
>>>>
>>>> Two queries I do have in V4L2-land:
>>>> - When s_dv_timings or s_std is called, is the format meant to
>>>> be updated automatically?
>>>
>>>
>>>
>>> Yes. Exception is if the new timings/std is exactly the same as the old
>>> timings/std, in that case you can just return 0 and do nothing.
>>
>>
>> OK, can do that.
>>
>>>> Even if we're already streaming?
>>>
>>>
>>> That's not allowed. Return -EBUSY in that case.
>>
>>
>> Also reasonable.
>> So if the TC358743 flags a source change we have to stop streaming,
>> set the new timings (which will update the format), and start up again
>> with fresh buffers. That's what I was expecting, but wanted to
>> confirm.
>
>
> Correct. In theory there are ways around this provided the buffers are
> large enough to accommodate the new format size, but nobody actually
> supports that (might change in the not-to-distant future).
>
>>
>>>> Some existing drivers seem to, but others don't.
>>>> - With s_fmt, is sizeimage settable by the application in the same
>>>> way as bytesperline?
>>>
>>>
>>>
&

Re: [RFC 0/2] BCM283x Camera Receiver driver

[Dave Stevenson]

Hi Hans.

On 14 June 2017 at 16:42, Hans Verkuil <hverk...@xs4all.nl> wrote:
> Hi Dave,
>
> How does this driver relate to this staging driver:
>
> drivers/staging/vc04_services/bcm2835-camera/
>
> It's not obvious to me.

drivers/staging/vc04_services/bcm2835-camera/ is using the VideoCore
firmware to control Unicam, ISP, and all the tuner algorithms. The ARM
gets delivered fully processed buffers from the VideoCore side. The
firmware only has drivers for the Omnivision OV5647 and Sony IMX219
(and an unsupported one for the Toshiba TC358743).

This driver is solely the Unicam block, reading the data in over
CSI2/CCP2 from the sensor and writing it to memory. No ISP or control
loops.
Other than power management, this driver is running solely on the ARM
with no involvement from the VideoCore firmware.
The sensor driver is whatever suitable V4L2 subdevice driver you fancy
attaching (as long as it supports CSI2, or eventually CCP2).

> On 06/14/2017 05:15 PM, Dave Stevenson wrote:
>>
>> Hi All.
>>
>> This is adding a V4L2 subdevice driver for the CSI2/CCP2 camera
>> receiver peripheral on BCM283x, as used on Raspberry Pi.
>>
>> v4l2-compliance results depend on the sensor subdevice this is
>> connected to. It passes the basic tests cleanly with TC358743,
>> but objects with OV5647
>> fail: v4l2-test-controls.cpp(574): g_ext_ctrls does not support count == 0
>> Neither OV5647 nor Unicam support any controls.
>
>
> Are you compiling v4l2-compliance from the v4l-utils git repo? If not,
> then please do so and run again. The version packaged by distros tends
> to be seriously outdated.

Yes, I'm building from v4l-utils.git.
I updated within the last week, although you appear to have added 2
commits since (both CEC related).
I'm on "ef074cf media-ctl: add colorimetry support"

>> I must admit to not having got OV5647 to stream with the current driver
>> register settings. It works with a set of register settings for VGA RAW10.
>> I also have a couple of patches pending for OV5647, but would like to
>> understand the issues better before sending them out.
>>
>> Two queries I do have in V4L2-land:
>> - When s_dv_timings or s_std is called, is the format meant to
>>be updated automatically?
>
>
> Yes. Exception is if the new timings/std is exactly the same as the old
> timings/std, in that case you can just return 0 and do nothing.

OK, can do that.

>> Even if we're already streaming?
>
> That's not allowed. Return -EBUSY in that case.

Also reasonable.
So if the TC358743 flags a source change we have to stop streaming,
set the new timings (which will update the format), and start up again
with fresh buffers. That's what I was expecting, but wanted to
confirm.

>>Some existing drivers seem to, but others don't.
>> - With s_fmt, is sizeimage settable by the application in the same
>>way as bytesperline?
>
>
> No, the driver will fill in this field, overwriting anything the
> application put there.
>
> bytesperline IS settable, but most drivers will ignore what userspace
> did and overwrite this as well.
>
> Normally the driver knows about HW requirements and will set sizeimage
> to something that will work (e.g. make sure it is a multiple of 16 lines).

There are subtly different requirements in different hardware blocks :-(
eg Unicam needs bytesperline to be a multiple of 16 bytes,whilst the
ISP requires a multiple of 32.
The vertical padding is generally where we're doing software
processing on the VideoCore side as it's easier to just leave the the
16 way SIMD processor running all 16 ways, hence needing scratch space
to avoid reading beyond buffers.

The main consumer is likely to be the ISP and that doesn't need
vertical context, so I'll look at removing the requirement there
rather than forcing it in this driver.
As long as we can set bytesperline (which is already supported) then
that requirement of the ISP is already handled.

>> yavta allows you to specify it on the command
>>
>>line, whilst v4l2-ctl doesn't. Some of the other parts of the Pi
>>firmware have a requirement that the buffer is a multiple of 16 lines
>>high, which can be matched by V4L2 if we can over-allocate the
>>buffers by the app specifying sizeimage. But if I allow that,
>>then I get a v4l2-compliance failure as the size doesn't get
>>reset when switching from RGB3 to UYVY as it takes the request as
>>a request to over-allocate.
>>
>> Apologies if I've messed up in sending these patches - so many ways
>> to do something.
>
>
> It looks fine at a glance.
>
> I will probably review this on Friday or Monday. But I need some
> clarification
> of the difference between this and the staging driver first.

Thanks. Hopefully I've given you that clarification above.

I'll fix the s_dv_timings and s_std handling, and s_fmt of sizeimage,
but will wait for other review comments before sending a v2.

  Dave

[RFC 2/2] [media] bcm2835-unicam: Driver for CCP2/CSI2 camera interface

[Dave Stevenson]

Add driver for the Unicam camera receiver block on
BCM283x processors.

Signed-off-by: Dave Stevenson <dave.steven...@raspberrypi.org>
---
 drivers/media/platform/Kconfig   |1 +
 drivers/media/platform/Makefile  |2 +
 drivers/media/platform/bcm2835/Kconfig   |   14 +
 drivers/media/platform/bcm2835/Makefile  |3 +
 drivers/media/platform/bcm2835/bcm2835-unicam.c  | 2100 ++
 drivers/media/platform/bcm2835/vc4-regs-unicam.h |  257 +++
 6 files changed, 2377 insertions(+)
 create mode 100644 drivers/media/platform/bcm2835/Kconfig
 create mode 100644 drivers/media/platform/bcm2835/Makefile
 create mode 100644 drivers/media/platform/bcm2835/bcm2835-unicam.c
 create mode 100644 drivers/media/platform/bcm2835/vc4-regs-unicam.h

diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
index 8da521a..aa9 100644
--- a/drivers/media/platform/Kconfig
+++ b/drivers/media/platform/Kconfig
@@ -135,6 +135,7 @@ source "drivers/media/platform/am437x/Kconfig"
 source "drivers/media/platform/xilinx/Kconfig"
 source "drivers/media/platform/rcar-vin/Kconfig"
 source "drivers/media/platform/atmel/Kconfig"
+source "drivers/media/platform/bcm2835/Kconfig"
 
 config VIDEO_TI_CAL
tristate "TI CAL (Camera Adaptation Layer) driver"
diff --git a/drivers/media/platform/Makefile b/drivers/media/platform/Makefile
index 6bbdf94..9c5e412 100644
--- a/drivers/media/platform/Makefile
+++ b/drivers/media/platform/Makefile
@@ -81,3 +81,5 @@ obj-$(CONFIG_VIDEO_MEDIATEK_VCODEC)   += mtk-vcodec/
 obj-$(CONFIG_VIDEO_MEDIATEK_MDP)   += mtk-mdp/
 
 obj-$(CONFIG_VIDEO_MEDIATEK_JPEG)  += mtk-jpeg/
+
+obj-y  += bcm2835/
diff --git a/drivers/media/platform/bcm2835/Kconfig 
b/drivers/media/platform/bcm2835/Kconfig
new file mode 100644
index 000..9f9be9e
--- /dev/null
+++ b/drivers/media/platform/bcm2835/Kconfig
@@ -0,0 +1,14 @@
+# Broadcom VideoCore4 V4L2 camera support
+
+config VIDEO_BCM2835_UNICAM
+   tristate "Broadcom BCM2835 Unicam video capture driver"
+   depends on VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API
+   depends on ARCH_BCM2708 || ARCH_BCM2709 || ARCH_BCM2835 || COMPILE_TEST
+   select VIDEOBUF2_DMA_CONTIG
+   select V4L2_FWNODE
+   ---help---
+ Say Y here to enable V4L2 subdevice for CSI2 receiver.
+ This is a V4L2 subdevice that interfaces directly to the VC4 
peripheral.
+
+  To compile this driver as a module, choose M here. The module
+  will be called bcm2835-unicam.
diff --git a/drivers/media/platform/bcm2835/Makefile 
b/drivers/media/platform/bcm2835/Makefile
new file mode 100644
index 000..a98aba0
--- /dev/null
+++ b/drivers/media/platform/bcm2835/Makefile
@@ -0,0 +1,3 @@
+# Makefile for BCM2835 Unicam driver
+
+obj-$(CONFIG_VIDEO_BCM2835_UNICAM) += bcm2835-unicam.o
diff --git a/drivers/media/platform/bcm2835/bcm2835-unicam.c 
b/drivers/media/platform/bcm2835/bcm2835-unicam.c
new file mode 100644
index 000..26039da
--- /dev/null
+++ b/drivers/media/platform/bcm2835/bcm2835-unicam.c
@@ -0,0 +1,2100 @@
+/*
+ * BCM2835 Unicam capture Driver
+ *
+ * Copyright (C) 2017 - Raspberry Pi (Trading) Ltd.
+ *
+ * Dave Stevenson <dave.steven...@raspberrypi.org>
+ *
+ * Based on TI am437x driver by Benoit Parrot and Lad, Prabhakar and
+ * TI CAL camera interface driver by Benoit Parrot.
+ *
+ * This program is free software; you may redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; version 2 of the License.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
+ * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
+ * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
+ * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS
+ * BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN
+ * ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN
+ * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+ * SOFTWARE.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include "vc4-regs-unicam.h"
+
+#define UNICAM_MODULE_NAME "unicam"
+#define UNICAM_VERSION "0.1.0"
+
+static int debug;
+module_param(debug, int, 0644);
+MODULE_PARM_DESC(debug, "Debug level 0-3");
+
+#define unicam_dbg(level, dev, fmt, arg...)\
+   v4l2_dbg(level, debug, &(dev)->v4l2_dev, fmt, ##arg)
+#define unicam_info(dev, fmt, arg...)  \
+   v4l2_info(&(dev)->v4l2_dev, fmt

[RFC 0/2] BCM283x Camera Receiver driver

[Dave Stevenson]

Hi All.

This is adding a V4L2 subdevice driver for the CSI2/CCP2 camera
receiver peripheral on BCM283x, as used on Raspberry Pi.

v4l2-compliance results depend on the sensor subdevice this is
connected to. It passes the basic tests cleanly with TC358743,
but objects with OV5647
fail: v4l2-test-controls.cpp(574): g_ext_ctrls does not support count == 0
Neither OV5647 nor Unicam support any controls.

I must admit to not having got OV5647 to stream with the current driver
register settings. It works with a set of register settings for VGA RAW10.
I also have a couple of patches pending for OV5647, but would like to
understand the issues better before sending them out.

Two queries I do have in V4L2-land:
- When s_dv_timings or s_std is called, is the format meant to
  be updated automatically? Even if we're already streaming?
  Some existing drivers seem to, but others don't.
- With s_fmt, is sizeimage settable by the application in the same
  way as bytesperline? yavta allows you to specify it on the command
  line, whilst v4l2-ctl doesn't. Some of the other parts of the Pi
  firmware have a requirement that the buffer is a multiple of 16 lines
  high, which can be matched by V4L2 if we can over-allocate the
  buffers by the app specifying sizeimage. But if I allow that,
  then I get a v4l2-compliance failure as the size doesn't get
  reset when switching from RGB3 to UYVY as it takes the request as
  a request to over-allocate.

Apologies if I've messed up in sending these patches - so many ways
to do something.

Thanks in advance.
  Dave

Dave Stevenson (2):
  [media] dt-bindings: Document BCM283x CSI2/CCP2 receiver
  [media] bcm2835-unicam: Driver for CCP2/CSI2 camera interface

 .../devicetree/bindings/media/bcm2835-unicam.txt   |   76 +
 drivers/media/platform/Kconfig |1 +
 drivers/media/platform/Makefile|2 +
 drivers/media/platform/bcm2835/Kconfig |   14 +
 drivers/media/platform/bcm2835/Makefile|3 +
 drivers/media/platform/bcm2835/bcm2835-unicam.c| 2100 
 drivers/media/platform/bcm2835/vc4-regs-unicam.h   |  257 +++
 7 files changed, 2453 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/bcm2835-unicam.txt
 create mode 100644 drivers/media/platform/bcm2835/Kconfig
 create mode 100644 drivers/media/platform/bcm2835/Makefile
 create mode 100644 drivers/media/platform/bcm2835/bcm2835-unicam.c
 create mode 100644 drivers/media/platform/bcm2835/vc4-regs-unicam.h

-- 
2.7.4

[RFC 1/2] [media] dt-bindings: Document BCM283x CSI2/CCP2 receiver

[Dave Stevenson]

Document the DT bindings for the CSI2/CCP2 receiver peripheral
(known as Unicam) on BCM283x SoCs.

Signed-off-by: Dave Stevenson <dave.steven...@raspberrypi.org>
---
 .../devicetree/bindings/media/bcm2835-unicam.txt   | 76 ++
 1 file changed, 76 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/bcm2835-unicam.txt

diff --git a/Documentation/devicetree/bindings/media/bcm2835-unicam.txt 
b/Documentation/devicetree/bindings/media/bcm2835-unicam.txt
new file mode 100644
index 000..cc5a451
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/bcm2835-unicam.txt
@@ -0,0 +1,76 @@
+Broadcom BCM283x Camera Interface (Unicam)
+--
+
+The Unicam block on BCM283x SoCs is the receiver for either
+CSI-2 or CCP2 data from image sensors or similar devices.
+
+Required properties:
+===
+- compatible   : must be "brcm,bcm2835-unicam".
+- reg  : physical base address and length of the register sets for the
+ device.
+- interrupts   : should contain the IRQ line for this Unicam instance.
+- clocks   : list of clock specifiers, corresponding to entries in
+ clock-names property.
+- clock-names  : must contain an "lp_clock" entry, matching entries
+ in the clocks property.
+
+Optional properties
+===
+- max-data-lanes: the hardware can support varying numbers of clock lanes.
+ This value is the maximum number supported by this instance.
+ Known values of 2 or 4. Default is 2.
+
+
+Unicam supports a single port node. It should contain one 'port' child node
+with child 'endpoint' node. Please refer to the bindings defined in
+Documentation/devicetree/bindings/media/video-interfaces.txt.
+
+Example:
+   csi1: csi@7e801000 {
+   compatible = "brcm,bcm2835-unicam";
+   reg = <0x7e801000 0x800>,
+ <0x7e802004 0x4>;
+   interrupts = <2 7>;
+   clocks = < BCM2835_CLOCK_CAM1>;
+   clock-names = "lp_clock";
+
+   port {
+   #address-cells = <1>;
+   #size-cells = <0>;
+
+   endpoint {
+   remote-endpoint = <_0>;
+
+   };
+   };
+   };
+
+   i2c0: i2c@7e205000 {
+
+   tc358743: tc358743@0f {
+   compatible = "toshiba,tc358743";
+   reg = <0x0f>;
+   status = "okay";
+
+   clocks = <_clk>;
+   clock-names = "refclk";
+
+   tc358743_clk: bridge-clk {
+   compatible = "fixed-clock";
+   #clock-cells = <0>;
+   clock-frequency = <2700>;
+   };
+
+   port {
+   tc358743_0: endpoint {
+   remote-endpoint = <>;
+   clock-lanes = <0>;
+   data-lanes = <1 2 3 4>;
+   clock-noncontinuous;
+   link-frequencies =
+   /bits/ 64 <29700>;
+   };
+   };
+   };
+   };
-- 
2.7.4

Re: [PATCH 0/3] tc358743: minor driver fixes

[Dave Stevenson]

On 2 June 2017 at 15:13, Philipp Zabel <p.za...@pengutronix.de> wrote:
> On Fri, 2017-06-02 at 15:27 +0200, Hans Verkuil wrote:
>> On 06/02/17 15:03, Dave Stevenson wrote:
>> > Hi Hans.
>> >
>> > On 2 June 2017 at 13:35, Hans Verkuil <hverk...@xs4all.nl> wrote:
>> >> On 06/02/17 14:18, Dave Stevenson wrote:
>> >>> These 3 patches for TC358743 came out of trying to use the
>> >>> existing driver with a new Raspberry Pi CSI-2 receiver driver.
>> >>
>> >> Nice! Doing that has been on my todo list for ages but I never got
>> >> around to it. I have one of these and using the Raspberry Pi with
>> >> the tc358743 would allow me to add a CEC driver as well.
>> >
>> > It's been on my list for a while too! It's working, but just the final
>> > clean ups needed.
>> > I've got 1 v4l2-compliance failure still outstanding that needs
>> > digging into (subscribe_event), rebasing on top of the fwnode tree,
>> > and a couple of config things to tidy up. RFC hopefully next week.
>> > I'm testing with a demo board designed here at Pi Towers, but there
>> > are others successfully testing it using the auvidea.com B101 board.
>> >
>> > Are you aware of the HDMI modes that the TC358743 driver has been used 
>> > with?
>> > The comments mention 720P60 at 594MHz, but I have had to modify the
>> > fifo_level value from 16 to 110 to get VGA60 or 576P50 to work. (The
>> > value came out of Toshiba's spreadsheet for computing register
>> > settings). It increases the delay by 2.96usecs at 720P60 on 2 lanes,
>> > so not a huge change.
>> > Is it worth going to the effort of dynamically computing the delay, or
>> > is increasing the default acceptable?
>>
>> I see that the fifo_level value of 16 was supplied by Philipp Zabel, so
>> I have CC-ed him as I am not sure where those values came from.
>
> I've just chosen a small delay that worked reliably. For 4-lane 1080p60
> and for 2-lane 720p60 at 594 Mbps lane speed, the Toshiba spreadsheet
> believes that it is ok to decrease the FIFO delay all the way down to 0
> (it is not). I think it should be fine to delay transmission for a few
> microseconds unconditionally, I'll test this next week.

Thanks Philipp. Were 1080p60 and 720p60 the only modes you were really testing?

Did the 594Mbps lane speed come from a specific requirement somewhere?
The standard Pi only has 2 CSI2 lanes exposed, and 1080P30 RGB3 just
tips over into needing 3 lanes with the current link frequency. When I
can find a bit more time I was thinking that an alternate link
frequency would allow us to squeeze it in to 2 lanes. Obviously the
timing values need to be checked carefully, but it should all work and
allow support of multiple link frequencies.
(My calcs say that 1080p50 UYVY can fit down 2 lanes, but that
requires more extensive register mods).

>> This driver is also used in a Cisco product, but we use platform_data for 
>> that.
>> Here are our settings that we use for reference:
>>
>> static struct tc358743_platform_data tc358743_pdata = {
>> .refclk_hz = 2700,
>> .ddc5v_delay = DDC5V_DELAY_100_MS,
>> .fifo_level = 300,
>> .pll_prd = 4,
>> .pll_fbd = 122,
>> /* CSI */
>> .lineinitcnt = 0x1770,
>> .lptxtimecnt = 0x0005,
>> .tclk_headercnt = 0x1d04,
>> .ths_headercnt = 0x0505,
>> .twakeup = 0x4650,
>> .ths_trailcnt = 0x0004,
>> .hstxvregcnt = 0x0005,
>> /* HDMI PHY */
>> .hdmi_phy_auto_reset_tmds_detected = true,
>> .hdmi_phy_auto_reset_tmds_in_range = true,
>> .hdmi_phy_auto_reset_tmds_valid = true,
>> .hdmi_phy_auto_reset_hsync_out_of_range = true,
>> .hdmi_phy_auto_reset_vsync_out_of_range = true,
>> .hdmi_detection_delay = HDMI_MODE_DELAY_25_MS,
>> };
>>
>> I believe these are all calculated from the Toshiba spreadsheet.
>>
>> Frankly, I have no idea what they mean :-)
>>
>> I am fine with increasing the default if Philipp is OK as well. Since
>> Cisco uses a value of 300 I would expect that 16 is indeed too low.
>>
>> Regards,
>>
>>   Hans
>>
>> >
>> >>> A couple of the subdevice API calls were not implemented or
>> >>> otherwise gave odd r

Re: [PATCH 0/3] tc358743: minor driver fixes

[Dave Stevenson]

Hi Hans.

On 2 June 2017 at 13:35, Hans Verkuil <hverk...@xs4all.nl> wrote:
> On 06/02/17 14:18, Dave Stevenson wrote:
>> These 3 patches for TC358743 came out of trying to use the
>> existing driver with a new Raspberry Pi CSI-2 receiver driver.
>
> Nice! Doing that has been on my todo list for ages but I never got
> around to it. I have one of these and using the Raspberry Pi with
> the tc358743 would allow me to add a CEC driver as well.

It's been on my list for a while too! It's working, but just the final
clean ups needed.
I've got 1 v4l2-compliance failure still outstanding that needs
digging into (subscribe_event), rebasing on top of the fwnode tree,
and a couple of config things to tidy up. RFC hopefully next week.
I'm testing with a demo board designed here at Pi Towers, but there
are others successfully testing it using the auvidea.com B101 board.

Are you aware of the HDMI modes that the TC358743 driver has been used with?
The comments mention 720P60 at 594MHz, but I have had to modify the
fifo_level value from 16 to 110 to get VGA60 or 576P50 to work. (The
value came out of Toshiba's spreadsheet for computing register
settings). It increases the delay by 2.96usecs at 720P60 on 2 lanes,
so not a huge change.
Is it worth going to the effort of dynamically computing the delay, or
is increasing the default acceptable?

>> A couple of the subdevice API calls were not implemented or
>> otherwise gave odd results. Those are fixed.
>>
>> The TC358743 interface board being used didn't have the IRQ
>> line wired up to the SoC. "interrupts" is listed as being
>> optional in the DT binding, but the driver didn't actually
>> function if it wasn't provided.
>>
>> Dave Stevenson (3):
>>   [media] tc358743: Add enum_mbus_code
>>   [media] tc358743: Setup default mbus_fmt before registering
>>   [media] tc358743: Add support for platforms without IRQ line
>
> All looks good, I'll take this for 4.12.

Thanks.

> Regards,
>
> Hans
>
>>
>>  drivers/media/i2c/tc358743.c | 59 
>> +++-
>>  1 file changed, 58 insertions(+), 1 deletion(-)
>>
>

[PATCH 2/3] [media] tc358743: Setup default mbus_fmt before registering

[Dave Stevenson]

Previously the mbus_fmt_code was set after the device was
registered. If a connected sub-device called tc358743_get_fmt
prior to that point it would get an invalid code back.

Signed-off-by: Dave Stevenson <dave.steven...@raspberrypi.org>
---
 drivers/media/i2c/tc358743.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/media/i2c/tc358743.c b/drivers/media/i2c/tc358743.c
index 06bfdca..2f5763d 100644
--- a/drivers/media/i2c/tc358743.c
+++ b/drivers/media/i2c/tc358743.c
@@ -1905,6 +1905,8 @@ static int tc358743_probe(struct i2c_client *client,
if (err < 0)
goto err_hdl;
 
+   state->mbus_fmt_code = MEDIA_BUS_FMT_RGB888_1X24;
+
sd->dev = >dev;
err = v4l2_async_register_subdev(sd);
if (err < 0)
@@ -1919,7 +1921,6 @@ static int tc358743_probe(struct i2c_client *client,
 
tc358743_s_dv_timings(sd, _timing);
 
-   state->mbus_fmt_code = MEDIA_BUS_FMT_RGB888_1X24;
tc358743_set_csi_color_space(sd);
 
tc358743_init_interrupts(sd);
-- 
2.7.4

[PATCH 1/3] [media] tc358743: Add enum_mbus_code

[Dave Stevenson]

There was no way to query the supported mbus formats from this
driver. enum_mbus_code is the function to expose that, so
implement it.

Signed-off-by: Dave Stevenson <dave.steven...@raspberrypi.org>
---
 drivers/media/i2c/tc358743.c | 18 ++
 1 file changed, 18 insertions(+)

diff --git a/drivers/media/i2c/tc358743.c b/drivers/media/i2c/tc358743.c
index 3251cba..06bfdca 100644
--- a/drivers/media/i2c/tc358743.c
+++ b/drivers/media/i2c/tc358743.c
@@ -1473,6 +1473,23 @@ static int tc358743_s_stream(struct v4l2_subdev *sd, int 
enable)
 
 /* --- PAD OPS --- */
 
+static int tc358743_enum_mbus_code(struct v4l2_subdev *sd,
+   struct v4l2_subdev_pad_config *cfg,
+   struct v4l2_subdev_mbus_code_enum *code)
+{
+   switch (code->index) {
+   case 0:
+   code->code = MEDIA_BUS_FMT_RGB888_1X24;
+   break;
+   case 1:
+   code->code = MEDIA_BUS_FMT_UYVY8_1X16;
+   break;
+   default:
+   return -EINVAL;
+   }
+   return 0;
+}
+
 static int tc358743_get_fmt(struct v4l2_subdev *sd,
struct v4l2_subdev_pad_config *cfg,
struct v4l2_subdev_format *format)
@@ -1642,6 +1659,7 @@ static const struct v4l2_subdev_video_ops 
tc358743_video_ops = {
 };
 
 static const struct v4l2_subdev_pad_ops tc358743_pad_ops = {
+   .enum_mbus_code = tc358743_enum_mbus_code,
.set_fmt = tc358743_set_fmt,
.get_fmt = tc358743_get_fmt,
.get_edid = tc358743_g_edid,
-- 
2.7.4

[PATCH 3/3] [media] tc358743: Add support for platforms without IRQ line

[Dave Stevenson]

interrupts is listed as an optional property in the DT
binding, but in reality the driver didn't work without it.
The existing driver relied on having the interrupt line
connected to the SoC to trigger handling various events.

Add the option to poll the interrupt status register via
a timer if no interrupt source is defined.

Signed-off-by: Dave Stevenson <dave.steven...@raspberrypi.org>
---
 drivers/media/i2c/tc358743.c | 38 ++
 1 file changed, 38 insertions(+)

diff --git a/drivers/media/i2c/tc358743.c b/drivers/media/i2c/tc358743.c
index 2f5763d..654aac2 100644
--- a/drivers/media/i2c/tc358743.c
+++ b/drivers/media/i2c/tc358743.c
@@ -33,6 +33,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -61,6 +62,8 @@ MODULE_LICENSE("GPL");
 
 #define I2C_MAX_XFER_SIZE  (EDID_BLOCK_SIZE + 2)
 
+#define POLL_INTERVAL_MS   1000
+
 static const struct v4l2_dv_timings_cap tc358743_timings_cap = {
.type = V4L2_DV_BT_656_1120,
/* keep this initialization for compatibility with GCC < 4.4.6 */
@@ -91,6 +94,9 @@ struct tc358743_state {
 
struct delayed_work delayed_work_enable_hotplug;
 
+   struct timer_list timer;
+   struct work_struct work_i2c_poll;
+
/* edid  */
u8 edid_blocks_written;
 
@@ -1319,6 +1325,24 @@ static irqreturn_t tc358743_irq_handler(int irq, void 
*dev_id)
return handled ? IRQ_HANDLED : IRQ_NONE;
 }
 
+static void tc358743_irq_poll_timer(unsigned long arg)
+{
+   struct tc358743_state *state = (struct tc358743_state *)arg;
+
+   schedule_work(>work_i2c_poll);
+
+   mod_timer(>timer, jiffies + msecs_to_jiffies(POLL_INTERVAL_MS));
+}
+
+static void tc358743_work_i2c_poll(struct work_struct *work)
+{
+   struct tc358743_state *state = container_of(work,
+   struct tc358743_state, work_i2c_poll);
+   bool handled;
+
+   tc358743_isr(>sd, 0, );
+}
+
 static int tc358743_subscribe_event(struct v4l2_subdev *sd, struct v4l2_fh *fh,
struct v4l2_event_subscription *sub)
 {
@@ -1933,6 +1957,14 @@ static int tc358743_probe(struct i2c_client *client,
"tc358743", state);
if (err)
goto err_work_queues;
+   } else {
+   INIT_WORK(>work_i2c_poll,
+ tc358743_work_i2c_poll);
+   state->timer.data = (unsigned long)state;
+   state->timer.function = tc358743_irq_poll_timer;
+   state->timer.expires = jiffies +
+  msecs_to_jiffies(POLL_INTERVAL_MS);
+   add_timer(>timer);
}
 
tc358743_enable_interrupts(sd, tx_5v_power_present(sd));
@@ -1948,6 +1980,8 @@ static int tc358743_probe(struct i2c_client *client,
return 0;
 
 err_work_queues:
+   if (!state->i2c_client->irq)
+   flush_work(>work_i2c_poll);
cancel_delayed_work(>delayed_work_enable_hotplug);
mutex_destroy(>confctl_mutex);
 err_hdl:
@@ -1961,6 +1995,10 @@ static int tc358743_remove(struct i2c_client *client)
struct v4l2_subdev *sd = i2c_get_clientdata(client);
struct tc358743_state *state = to_state(sd);
 
+   if (!state->i2c_client->irq) {
+   del_timer_sync(>timer);
+   flush_work(>work_i2c_poll);
+   }
cancel_delayed_work(>delayed_work_enable_hotplug);
v4l2_async_unregister_subdev(sd);
v4l2_device_unregister_subdev(sd);
-- 
2.7.4

[PATCH 0/3] tc358743: minor driver fixes

[Dave Stevenson]

These 3 patches for TC358743 came out of trying to use the
existing driver with a new Raspberry Pi CSI-2 receiver driver.

A couple of the subdevice API calls were not implemented or
otherwise gave odd results. Those are fixed.

The TC358743 interface board being used didn't have the IRQ
line wired up to the SoC. "interrupts" is listed as being
optional in the DT binding, but the driver didn't actually
function if it wasn't provided.

Dave Stevenson (3):
  [media] tc358743: Add enum_mbus_code
  [media] tc358743: Setup default mbus_fmt before registering
  [media] tc358743: Add support for platforms without IRQ line

 drivers/media/i2c/tc358743.c | 59 +++-
 1 file changed, 58 insertions(+), 1 deletion(-)

-- 
2.7.4

Re: Sensor sub-device - what are the mandatory ops?

[Dave Stevenson]

Hi Sakari .
Thanks for taking the time to respond.

On 17 May 2017 at 00:04, Sakari Ailus <sakari.ai...@iki.fi> wrote:
> Hi Dave,
>
> On Thu, May 11, 2017 at 04:51:27PM +0100, Dave Stevenson wrote:
>> Hi All.
>>
>> As previously discussed, I'm working on a V4L2 driver for the
>> CSI-2/CCP2 receiver on BCM283x, as used on Raspberry Pi.
>> It's a relatively simple hardware block that writes received data into
>> SDRAM, and only accepts connection from one "sensor" sub device, so no
>> need to involve the media controller API. (The peripheral can do
>> cropping and format conversion between the CSI-2 Bayer formats too,
>> but I'm ignoring those for now, and even so they don't really need
>> media controller).
>
> If the sensor exposes multiple sub-devices or you have e.g. a more complex
> TV tuner with internal data routing such as some of the ADV chips, you
> won't be able to support them in this model.
>
> On the other hand, using Media controller brings some extra complexity and
> requires generally a lot more decision making and configuration from the
> user space as well. (This is something that needs to be more or less
> resolved over time but how long it'll take I can't say.)
>
> Just FYI.

Thanks for the warning.
I was aware that I might be limiting the use of some drivers, but I'm
also aware that the majority of users on the Pi are not "power users",
and if it takes much more than opening /dev/video0 then they'll lose
interest.
The CSI-2 sensor drivers that I've looked at haven't had vast
complexity. As and when any get added we can look at adding in media
controller support.

>> I was previously advised by Hans to take am437x as a base, and that
>> seems to have worked pretty well when combined with some of the ti-vpe
>> driver too. It's up and running, although with some rough edges. I'm
>> hoping to sort an RFC in a week or so.
>>
>> My main issue is determining what calls are mandatory to be supported
>> by the sensor sub-device drivers that attach to the CSI-2 receiver.
>> I'm either taking the wrong approach, or there seem to be missing ops
>> in the drivers I'm trying to use. The set of devices I have available
>> are Omnivision OV5647, Toshiba TC358743 HDMI to CSI2 bridge, and
>> ADV7282-M analogue video to CSI-2 decoder.
>>
>> The TC358743 driver doesn't support:
>> - enum_mbus_code to report the supported formats
>> (MEDIA_BUS_FMT_RGB888_1X24 and MEDIA_BUS_FMT_UYVY8_1X16)
>
> Without knowing any details on the chip nor its driver, if it supports the
> two, it'd be reasonable that it also provided such enumeration of mbus
> codes.

It does support both formats, so, I'll submit that patch.

>> - s_power. The docs [1] say the device must be powered up before
>> calling v4l2_subdev_video_ops->s_stream, but is s_power optional so
>> ENOIOCTLCMD is not to be considered a failure?
>
> Correct.

Easy enough to handle.

>> - enum_frame_size
>> and doesn't set the state->mbus_fmt_code until after
>> v4l2_async_register_subdev. A connected subdevice calling get_fmt
>> during the notifier.complete callback gets a code of 0.
>>
>> The OV5647 driver doesn't support:
>> - set_fmt or get_fmt. I can't see any code that returns the 640x480
>> sensor resolution that is listed in the commit text.
>
> get_fmt is essential. Without that link validation can't work. If the driver
> doesn't support setting the format, the get_fmt callback can be used for
> set_fmt as well.

Another patch to submit then.
Using get_fmt in the absence of set_fmt feels a little nasty, but
probably a sensible precaution.

>> - g_mbus_config, so no information on the number of CSI-2 lanes in use
>> beyond that in DT. Do we just assume all lines specified in DT are in
>> use in this situation? In which case should the driver be checking
>> that the configured number of lanes matches the register set it will
>> request over I2C, as a mismatch will result in it not working?
>
> The assumption is that all lanes are in use. g_mbus_config is from SoC
> camera and, well, frankly should not look like how it looks now. It's used
> by a couple of drivers and needs to be replaced by more generic and
> informative means to let the receiver know the frame structure the
> transmitter is about to start sending.

So it is what it is, but isn't mandatory.
Dropping it is likely to cause issues with the TC358743. It receives
HDMI data in at whatever rate is required for the format, stores it in
a small FIFO, and when a trigger point is reached in the FIFO it is
read out over CSI2 with N data lanes at a defined rate. Under or over
flow that FIFO and you get corrupted images. It dynamically 

Sensor sub-device - what are the mandatory ops?

[Dave Stevenson]

Hi All.

As previously discussed, I'm working on a V4L2 driver for the
CSI-2/CCP2 receiver on BCM283x, as used on Raspberry Pi.
It's a relatively simple hardware block that writes received data into
SDRAM, and only accepts connection from one "sensor" sub device, so no
need to involve the media controller API. (The peripheral can do
cropping and format conversion between the CSI-2 Bayer formats too,
but I'm ignoring those for now, and even so they don't really need
media controller).
I was previously advised by Hans to take am437x as a base, and that
seems to have worked pretty well when combined with some of the ti-vpe
driver too. It's up and running, although with some rough edges. I'm
hoping to sort an RFC in a week or so.

My main issue is determining what calls are mandatory to be supported
by the sensor sub-device drivers that attach to the CSI-2 receiver.
I'm either taking the wrong approach, or there seem to be missing ops
in the drivers I'm trying to use. The set of devices I have available
are Omnivision OV5647, Toshiba TC358743 HDMI to CSI2 bridge, and
ADV7282-M analogue video to CSI-2 decoder.

The TC358743 driver doesn't support:
- enum_mbus_code to report the supported formats
(MEDIA_BUS_FMT_RGB888_1X24 and MEDIA_BUS_FMT_UYVY8_1X16)
- s_power. The docs [1] say the device must be powered up before
calling v4l2_subdev_video_ops->s_stream, but is s_power optional so
ENOIOCTLCMD is not to be considered a failure?
- enum_frame_size
and doesn't set the state->mbus_fmt_code until after
v4l2_async_register_subdev. A connected subdevice calling get_fmt
during the notifier.complete callback gets a code of 0.

The OV5647 driver doesn't support:
- set_fmt or get_fmt. I can't see any code that returns the 640x480
sensor resolution that is listed in the commit text.
- g_mbus_config, so no information on the number of CSI-2 lanes in use
beyond that in DT. Do we just assume all lines specified in DT are in
use in this situation? In which case should the driver be checking
that the configured number of lanes matches the register set it will
request over I2C, as a mismatch will result in it not working?
- enum_frame_size

ADV7180/7282-M
- enum_frame_size

I've listed enum_frame_size as that is what TI VPE driver uses in
cal_try_fmt_vid_cap. It seems more sensible to pass the request in to
set_fmt with which = V4L2_SUBDEV_FORMAT_TRY, so is this actually an
issue with the TI driver doing the wrong thing? (FORMAT_TRY seems to
work reasonably).

Those are the issues I've hit on those 3 drivers. Is there a defintive
list of what must be supported by drivers, and any checklist for
drivers during review?

I have patches for the TC358743 and OV5647 which I can post to the
list if it is agreed that the above are issues rather than me doing
the wrong thing.


Follow-up question on g_mbus_format. The V4L2_MBUS_CSI2_x_LANE defines
appear to have been specfied though they should be used as a bitmask,
but based on existing drivers (mainly TC358743) only one is allowed to
be set to denote the actual number of lanes used. Is that the correct
interpretation? If so I guess we need error checking on the flags
passed in.


One last question. Putting a user's hat on, what is the expected
mapping of vidioc_s_input to s_routing?
Looking at my use case, the CSI-2 receiver driver is the code that
creates /dev/videoN, but it otherwise is just a proxy for the sensor
device. It therefore makes the user's life easy if calls such as
input, EDID, dv_timings, and std functions are just passed straight
through to the sensor, so the user can ignore the subdev API.
For input there appears to be no way to produce an implementation of
vidioc_enum_input. Looking at the ADV7282-M (uses ADV7180 driver), I
can't see any way of reading out the valid input numbers as would be
needed for enum_input.
vidioc_g_input can be done by the CSI-2 receiver driver
assuming/setting to input 0 during probe, and then caching the last
set value, but that feels a little nasty. Have I missed something
there?

Thanks in advance.
  Dave

[1] 
https://linuxtv.org/downloads/v4l-dvb-apis-new/kapi/csi2.html#receiver-drivers

Re: [PATCH 1/6] staging: Import the BCM2835 MMAL-based V4L2 camera driver.

[Dave Stevenson]

Hi Hans.

On 06/02/17 12:58, Hans Verkuil wrote:

On 02/06/2017 12:37 PM, Dave Stevenson wrote:

Hi Hans.

On 06/02/17 09:08, Hans Verkuil wrote:

Hi Eric,

Great to see this driver appearing for upstream merging!

See below for my review comments, focusing mostly on V4L2 specifics.






+   f->fmt.pix.pixelformat = dev->capture.fmt->fourcc;
+   f->fmt.pix.bytesperline = dev->capture.stride;
+   f->fmt.pix.sizeimage = dev->capture.buffersize;
+
+   if (dev->capture.fmt->fourcc == V4L2_PIX_FMT_RGB24)
+   f->fmt.pix.colorspace = V4L2_COLORSPACE_SRGB;
+   else if (dev->capture.fmt->fourcc == V4L2_PIX_FMT_JPEG)
+   f->fmt.pix.colorspace = V4L2_COLORSPACE_JPEG;
+   else
+   f->fmt.pix.colorspace = V4L2_COLORSPACE_SMPTE170M;


Colorspace has nothing to do with the pixel format. It should come from the
sensor/video receiver.

If this information is not available, then COLORSPACE_SRGB is generally a
good fallback.


I would if I could, but then I fail v4l2-compliance on V4L2_PIX_FMT_JPEG
https://git.linuxtv.org/v4l-utils.git/tree/utils/v4l2-compliance/v4l2-test-formats.cpp#n329
The special case for JPEG therefore has to remain.


Correct. Sorry, my fault, I forgot about that.



It looks like I tripped over the subtlety between V4L2_COLORSPACE_,
V4L2_XFER_FUNC_, V4L2_YCBCR_ENC_, and V4L2_QUANTIZATION_, and Y'CbCr
encoding vs colourspace.

The ISP coefficients are set up for BT601 limited range, and any
conversion back to RGB is done based on that. That seemed to fit
SMPTE170M rather than SRGB.


Colorspace refers to the primary colors + whitepoint that are used to
create the colors (basically this answers the question to which colors
R, G and B exactly refer to). The SMPTE170M has different primaries
compared to sRGB (and a different default transfer function as well).

RGB vs Y'CbCr is just an encoding and it doesn't change the underlying
colorspace. Unfortunately, the term 'colorspace' is often abused to just
refer to RGB vs Y'CbCr.

If the colorspace is SRGB, then when the pixelformat is a Y'CbCr encoding,
then the BT601 limited range encoding is implied, unless overridden via
the ycbcr_enc and/or quantization fields in struct v4l2_pix_format.

In other words, this does already the right thing.


https://linuxtv.org/downloads/v4l-dvb-apis-new/uapi/v4l/pixfmt-007.html#colorspace-srgb-v4l2-colorspace-srgb
"The default transfer function is V4L2_XFER_FUNC_SRGB. The default 
Y’CbCr encoding is V4L2_YCBCR_ENC_601. The default Y’CbCr quantization 
is full range."

So full range or limited?


The JPEG colorspace is a short-hand for V4L2_COLORSPACE_SRGB, V4L2_YCBCR_ENC_601
and V4L2_QUANTIZATION_FULL_RANGE. It's historical that this colorspace exists.

If I would redesign this this JPEG colorspace would be dropped.

For a lot more colorspace information see:

https://hverkuil.home.xs4all.nl/spec/uapi/v4l/colorspaces.html



I do note that as there is now support for more RGB formats (BGR24 and
BGR32) the first "if" needs extending to cover those. Or I don't care
and only special case JPEG with all others just reporting SRGB.



Only special case JPEG.

But as I said, this information really needs to come from the sensor or
video receiver since this driver has no knowledge of this.


OK.






This is IMHO unnecessarily complex.

My recommendation is that controls are added with a set of v4l2_ctrl_new_std* 
calls
or if you really want to by walking a struct v4l2_ctrl_config array and adding 
controls
via v4l2_ctrl_new_custom.

The s_ctrl is a switch that calls the 'setter' function.

No need for arrays, callbacks, etc. Just keep it simple.


I can look into that, but I'm not sure I fully follow what you are
suggesting.

In the current implementation things like V4L2_CID_SATURATION,
V4L2_CID_SHARPNESS, V4L2_CID_CONTRAST, and V4L2_CID_BRIGHTNESS all use
the one common ctrl_set_rational setter function because the only thing
different in setting is the MMAL_PARAMETER_xxx value. I guess that could
move into the common setter based on V4L2_CID_xxx, but then the control
configuration is split between multiple places which feels less well
contained.


See e.g. samples/v4l/v4l2-pci-skeleton.c: in the probe function (or in a
function called from there if there are a lot of controls) you add the
controls, and in s_ctrl you handle them.

But this is just my preference.

So in s_ctrl you would see something like this:

switch (ctrl->id) {
case V4L2_CID_SATURATION:
ctrl_set_rational(ctrl->val, MMAL_PARAMETER_SAT);
break;
case V4L2_CID_BRIGHTNESS:
ctrl_set_rational(ctrl->val, MMAL_PARAMETER_BRIGHTNESS);
break;
...
}


OK, thanks for the clarification. That can be done.






Final question: did you run v4l2-compliance over this driver? Before this 
driver can
be moved out of staging it should pass the compliance tests. Note: always 
compile
this test from the main r

Re: [PATCH 1/6] staging: Import the BCM2835 MMAL-based V4L2 camera driver.

[Dave Stevenson]

Hi Mauro.

Can I just say that I'm not attempting to upstream this, others are. 
I've just answered questions raised.


On 06/02/17 12:37, Mauro Carvalho Chehab wrote:

Em Sun, 5 Feb 2017 22:15:21 +
Dave Stevenson <linux-me...@destevenson.freeserve.co.uk> escreveu:


If the goal was to protect some IP related to the sensors, I guess
this is not going to protect anything, as there are recent driver
submissions on linux-media for the ov5647 driver:

https://patchwork.kernel.org/patch/9472441/

There are also open source drivers for the Sony imx219 camera
floating around for android and chromeOS:


https://chromium.googlesource.com/chromiumos/third_party/kernel/+/factory-ryu-6486.14.B-chromeos-3.14/drivers/media/i2c/soc_camera/imx219.c

https://android.googlesource.com/kernel/bcm/+/android-bcm-tetra-3.10-lollipop-wear-release/drivers/media/video/imx219.c

Plus, there's a datasheet (with another driver) available at:
https://github.com/rellimmot/Sony-IMX219-Raspberry-Pi-V2-CMOS

So, you're not really protecting IP here.


None of the ISP processing, control loops, or tuning are open.
Yes there are kernel drivers kicking around for OV5647 and IMX219 now, 
but very little will consume raw Bayer.

That is also Omnivision or Sony IP, not Broadcom IP.


If the goal is to protect some proprietary algorithm meant to enhance
the camera captured streams, doing things like (auto-focus, auto-white
adjustments, scaling, etc), and/or implementing codec encoders, you should,
instead, restructure such codecs as mem2mem V4L2 drivers. There are a bunch
of such codecs already for other SoC where such functions are implemented
on GPU.

If you add DMABUF capabilities and media controller to the capture
driver and to the mem2mem drivers, userspace can use the V4L2 APIs
to use those modules using the arrangements they need, without
performance impacts.

So, by obfuscating the code, you're not protecting anything. Just making
harder for RPi customers to use, as you're providing a driver that is
limited.








Greg was kick on merging it on staging ;) Anyway, the real review
will happen when the driver becomes ready to be promoted out of
staging. When you address the existing issues and get it ready to
merge, please send the patch with such changes to linux-media ML.
I'll do a full review on it by then.


Is that even likely given the dependence on VCHI? I wasn't expecting
VCHI to leave staging, which would force this to remain too.


I didn't analyze the VCHI driver. As I said before, if you rewrite
the driver in a way that the Kernel can actually see the sensors
via an I2C interface, you probably can get rid of the VCHI interface
for the capture part.

You could take a look on the other mem2mem drivers and see if are
there some way to provide an interface for the GPU encoders in a
similar way to what those drivers do.


I will be looking at a sub dev driver for just the CCP2/CSI1/CSI2 
receiver as Broadcom did manage to release (probably unintentionally) a 
bastardised soc-camera driver for it and therefore the info is in the 
public domain.


It would be possible to write a second sub dev driver that was similar 
to this in using VCHI/MMAL to create another mem2mem device to wrap the 
ISP, but that would just be an image processing pipe - control loops 
would all be elsewhere (userspace).
I haven't seen a sensible mechanism within V4L2 for exporting image 
statistics for AE, AWB, AF, or histograms to userspace so that 
appropriate algorithms can be run there. Have I missed them, or do they 
not exist?






That seems a terrible hack! let the user specify the resolution via
modprobe parameter... That should depend on the hardware capabilities
instead.


This is sitting on top of an OpenMaxIL style camera component (though
accessed via MMAL - long story, but basically MMAL removed a bundle of
the ugly/annoying parts of IL).
It has the extension above V1.1.2 that you have a preview port, video
capture port, and stills capture port. Stills captures have additional
processing stages to improve image quality, whilst video has to maintain
framerate.


I see.


If you're asked for YUV or RGB frame, how do you choose between video or
stills? That's what is being set with these parameters, not the sensor
resolution. Having independent stills and video processing options
doesn't appear to be something that is supported in V4L2, but I'm open
to suggestions.


At the capture stage:

Assuming that the user wants to use different resolutions for video and
stills (for example, you're seeing a real time video, then you "click"
to capture a still image), you can create two buffer groups, one for
low-res video and another one for high-res image. When the button is
clicked, it will stop the low-res stream, set the parameters for the
high-res image and capture it.

For post-processing stage:

Switch the media pipeline via the media controller adding the post
processing codecs that will enhance t

Re: [PATCH 1/6] staging: Import the BCM2835 MMAL-based V4L2 camera driver.

[Dave Stevenson]

Hi Hans.

On 06/02/17 09:08, Hans Verkuil wrote:

Hi Eric,

Great to see this driver appearing for upstream merging!

See below for my review comments, focusing mostly on V4L2 specifics.

On 01/27/2017 10:54 PM, Eric Anholt wrote:

- Supports raw YUV capture, preview, JPEG and H264.
- Uses videobuf2 for data transfer, using dma_buf.
- Uses 3.6.10 timestamping
- Camera power based on use
- Uses immutable input mode on video encoder

This code comes from the Raspberry Pi kernel tree (rpi-4.9.y) as of
a15ba877dab4e61ea3fc7b006e2a73828b083c52.

Signed-off-by: Eric Anholt 
---
 .../media/platform/bcm2835/bcm2835-camera.c| 2016 
 .../media/platform/bcm2835/bcm2835-camera.h|  145 ++
 drivers/staging/media/platform/bcm2835/controls.c  | 1345 +
 .../staging/media/platform/bcm2835/mmal-common.h   |   53 +
 .../media/platform/bcm2835/mmal-encodings.h|  127 ++
 .../media/platform/bcm2835/mmal-msg-common.h   |   50 +
 .../media/platform/bcm2835/mmal-msg-format.h   |   81 +
 .../staging/media/platform/bcm2835/mmal-msg-port.h |  107 ++
 drivers/staging/media/platform/bcm2835/mmal-msg.h  |  404 
 .../media/platform/bcm2835/mmal-parameters.h   |  689 +++
 .../staging/media/platform/bcm2835/mmal-vchiq.c| 1916 +++
 .../staging/media/platform/bcm2835/mmal-vchiq.h|  178 ++
 12 files changed, 7111 insertions(+)
 create mode 100644 drivers/staging/media/platform/bcm2835/bcm2835-camera.c
 create mode 100644 drivers/staging/media/platform/bcm2835/bcm2835-camera.h
 create mode 100644 drivers/staging/media/platform/bcm2835/controls.c
 create mode 100644 drivers/staging/media/platform/bcm2835/mmal-common.h
 create mode 100644 drivers/staging/media/platform/bcm2835/mmal-encodings.h
 create mode 100644 drivers/staging/media/platform/bcm2835/mmal-msg-common.h
 create mode 100644 drivers/staging/media/platform/bcm2835/mmal-msg-format.h
 create mode 100644 drivers/staging/media/platform/bcm2835/mmal-msg-port.h
 create mode 100644 drivers/staging/media/platform/bcm2835/mmal-msg.h
 create mode 100644 drivers/staging/media/platform/bcm2835/mmal-parameters.h
 create mode 100644 drivers/staging/media/platform/bcm2835/mmal-vchiq.c
 create mode 100644 drivers/staging/media/platform/bcm2835/mmal-vchiq.h

diff --git a/drivers/staging/media/platform/bcm2835/bcm2835-camera.c 
b/drivers/staging/media/platform/bcm2835/bcm2835-camera.c
new file mode 100644
index ..4f03949aecf3
--- /dev/null
+++ b/drivers/staging/media/platform/bcm2835/bcm2835-camera.c
@@ -0,0 +1,2016 @@





+static int start_streaming(struct vb2_queue *vq, unsigned int count)
+{
+   struct bm2835_mmal_dev *dev = vb2_get_drv_priv(vq);
+   int ret;
+   int parameter_size;
+
+   v4l2_dbg(1, bcm2835_v4l2_debug, >v4l2_dev, "%s: dev:%p\n",
+__func__, dev);
+
+   /* ensure a format has actually been set */
+   if (dev->capture.port == NULL)
+   return -EINVAL;


Standard mistake. If start_streaming returns an error, then it should call 
vb2_buffer_done
for all queued buffers with state VB2_BUF_STATE_QUEUED. Otherwise the buffer 
administration
gets unbalanced.


OK.
This is an error path that I'm not convinced can ever be followed, just 
defensive programming. It may be a candidate for just removing, but yes 
otherwise it needs to be fixed to do the right thing.



+
+   if (enable_camera(dev) < 0) {
+   v4l2_err(>v4l2_dev, "Failed to enable camera\n");
+   return -EINVAL;
+   }
+
+   /*init_completion(>capture.frame_cmplt); */
+
+   /* enable frame capture */
+   dev->capture.frame_count = 1;
+
+   /* if the preview is not already running, wait for a few frames for AGC
+* to settle down.
+*/
+   if (!dev->component[MMAL_COMPONENT_PREVIEW]->enabled)
+   msleep(300);
+
+   /* enable the connection from camera to encoder (if applicable) */
+   if (dev->capture.camera_port != dev->capture.port
+   && dev->capture.camera_port) {
+   ret = vchiq_mmal_port_enable(dev->instance,
+dev->capture.camera_port, NULL);
+   if (ret) {
+   v4l2_err(>v4l2_dev,
+"Failed to enable encode tunnel - error %d\n",
+ret);
+   return -1;


Use a proper error, not -1.


+   }
+   }
+
+   /* Get VC timestamp at this point in time */
+   parameter_size = sizeof(dev->capture.vc_start_timestamp);
+   if (vchiq_mmal_port_parameter_get(dev->instance,
+ dev->capture.camera_port,
+ MMAL_PARAMETER_SYSTEM_TIME,
+ >capture.vc_start_timestamp,
+ _size)) {
+   v4l2_err(>v4l2_dev,
+

Re: [PATCH 1/6] staging: Import the BCM2835 MMAL-based V4L2 camera driver.

[Dave Stevenson]

 mode 100644
index ..4f03949aecf3
--- /dev/null
+++ b/drivers/staging/media/platform/bcm2835/bcm2835-camera.c
@@ -0,0 +1,2016 @@
+/*
+ * Broadcom BM2835 V4L2 driver
+ *
+ * Copyright © 2013 Raspberry Pi (Trading) Ltd.
+ *
+ * This file is subject to the terms and conditions of the GNU General Public
+ * License.  See the file COPYING in the main directory of this archive
+ * for more details.
+ *
+ * Authors: Vincent Sanders <vincent.sand...@collabora.co.uk>
+ *  Dave Stevenson <dst...@broadcom.com>
+ *  Simon Mellor <simel...@broadcom.com>
+ *  Luke Diamand <lu...@broadcom.com>


All of these are now dead email addresses.
Mine could be updated to dave.steven...@raspberrypi.org, but the others 
should probably be deleted.



+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include "mmal-common.h"
+#include "mmal-encodings.h"
+#include "mmal-vchiq.h"
+#include "mmal-msg.h"
+#include "mmal-parameters.h"
+#include "bcm2835-camera.h"
+
+#define BM2835_MMAL_VERSION "0.0.2"
+#define BM2835_MMAL_MODULE_NAME "bcm2835-v4l2"
+#define MIN_WIDTH 32
+#define MIN_HEIGHT 32
+#define MIN_BUFFER_SIZE (80*1024)
+
+#define MAX_VIDEO_MODE_WIDTH 1280
+#define MAX_VIDEO_MODE_HEIGHT 720


Hmm... Doesn't the max resolution depend on the sensor?


+
+#define MAX_BCM2835_CAMERAS 2
+
+MODULE_DESCRIPTION("Broadcom 2835 MMAL video capture");
+MODULE_AUTHOR("Vincent Sanders");
+MODULE_LICENSE("GPL");
+MODULE_VERSION(BM2835_MMAL_VERSION);
+
+int bcm2835_v4l2_debug;
+module_param_named(debug, bcm2835_v4l2_debug, int, 0644);
+MODULE_PARM_DESC(bcm2835_v4l2_debug, "Debug level 0-2");
+
+#define UNSET (-1)
+static int video_nr[] = {[0 ... (MAX_BCM2835_CAMERAS - 1)] = UNSET };
+module_param_array(video_nr, int, NULL, 0644);
+MODULE_PARM_DESC(video_nr, "videoX start numbers, -1 is autodetect");
+
+static int max_video_width = MAX_VIDEO_MODE_WIDTH;
+static int max_video_height = MAX_VIDEO_MODE_HEIGHT;
+module_param(max_video_width, int, S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH);
+MODULE_PARM_DESC(max_video_width, "Threshold for video mode");
+module_param(max_video_height, int, S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH);
+MODULE_PARM_DESC(max_video_height, "Threshold for video mode");


That seems a terrible hack! let the user specify the resolution via
modprobe parameter... That should depend on the hardware capabilities
instead.


This is sitting on top of an OpenMaxIL style camera component (though 
accessed via MMAL - long story, but basically MMAL removed a bundle of 
the ugly/annoying parts of IL).
It has the extension above V1.1.2 that you have a preview port, video 
capture port, and stills capture port. Stills captures have additional 
processing stages to improve image quality, whilst video has to maintain 
framerate.


If you're asked for YUV or RGB frame, how do you choose between video or 
stills? That's what is being set with these parameters, not the sensor 
resolution. Having independent stills and video processing options 
doesn't appear to be something that is supported in V4L2, but I'm open 
to suggestions.
There were thoughts that they could be exposed as different /dev/videoN 
devices, but that then poses a quandry to the client app as to which 
node to open, so complicates the client significantly. On the plus side 
it would then allow for things like zero shutter lag captures, and 
stills during video, where you want multiple streams (apparently) 
simultaneously, but is that worth the complexity? The general view was no.



+
+/* Gstreamer bug https://bugzilla.gnome.org/show_bug.cgi?id=726521
+ * v4l2src does bad (and actually wrong) things when the vidioc_enum_framesizes
+ * function says type V4L2_FRMSIZE_TYPE_STEPWISE, which we do by default.
+ * It's happier if we just don't say anything at all, when it then
+ * sets up a load of defaults that it thinks might work.
+ * If gst_v4l2src_is_broken is non-zero, then we remove the function from
+ * our function table list (actually switch to an alternate set, but same
+ * result).
+ */
+static int gst_v4l2src_is_broken;
+module_param(gst_v4l2src_is_broken, int, S_IRUSR | S_IWUSR | S_IRGRP | 
S_IROTH);
+MODULE_PARM_DESC(gst_v4l2src_is_broken, "If non-zero, enable workaround for 
Gstreamer");


Not sure if I liked this hack here. AFAIKT, GStreamer fixed the bug with
V4L2_FRMSIZE_TYPE_STEPWISE already.


I will double check on Monday. The main Raspberry Pi distribution is 
based on Debian, so packages can be quite out of date. This bug 
certainly affected Wheezy, but I don't know for certain about Jessie. 
Sid still hasn't been adopted.


Also be aware that exactly the same issue of not supporting 
V4L2_FRMSIZE_TYPE_STEPWISE affects Chromium for WebRTC, and they

Re: uvcvideo logging kernel warnings on device disconnect

[Dave Stevenson]

Hi Greg.

On 09/12/16 09:43, Greg KH wrote:

On Fri, Dec 09, 2016 at 11:14:41AM +0200, Laurent Pinchart wrote:

Hi Greg,

On Friday 09 Dec 2016 10:11:13 Greg KH wrote:

On Fri, Dec 09, 2016 at 10:59:24AM +0200, Laurent Pinchart wrote:

On Friday 09 Dec 2016 08:25:52 Greg KH wrote:

On Fri, Dec 09, 2016 at 01:09:21AM +0200, Laurent Pinchart wrote:

On Thursday 08 Dec 2016 12:31:55 Dave Stevenson wrote:

Hi All.

I'm working with a USB webcam which has been seen to spontaneously
disconnect when in use. That's a separate issue, but when it does it
throws a load of warnings into the kernel log if there is a file
handle on the device open at the time, even if not streaming.

I've reproduced this with a generic Logitech C270 webcam on:
- Ubuntu 16.04 (kernel 4.4.0-51) vanilla, and with the latest media
tree from linuxtv.org
- Ubuntu 14.04 (kernel 4.4.0-42) vanilla
- an old 3.10.x tree on an embedded device.

To reproduce:
- connect USB webcam.
- run a simple app that opens /dev/videoX, sleeps for a while, and
then closes the handle.
- disconnect the webcam whilst the app is running.
- read kernel logs - observe warnings. We get the disconnect logged
as it occurs, but the warnings all occur when the file descriptor is
closed. (A copy of the logs from my Ubuntu 14.04 machine are below).

I can fully appreciate that the open file descriptor is holding
references to a now invalid device, but is there a way to avoid them?
Or do we really not care and have to put up with the log noise when
doing such silly things?


This is a known problem, caused by the driver core trying to remove
the same sysfs attributes group twice.


Ick, not good.


The group is first removed when the USB device is disconnected. The
input device and media device created by the uvcvideo driver are
children of the USB interface device, which is deleted from the system
when the camera is unplugged. Due to the parent-child relationship,
all sysfs attribute groups of the children are removed.


Wait, why is the USB device being removed from sysfs at this point,
didn't the input and media subsystems grab a reference to it so that it
does not disappear just yet?


References are taken in uvc_prove():
dev->udev = usb_get_dev(udev);
dev->intf = usb_get_intf(intf);


s/uvc_prove/uvc_probe/ ?  :)


Oops :-)


and released in uvc_delete(), called when the last video device node is
closed. This prevents the device from being released (freed), but
device_del() is synchronous to device unplug as far as I understand.


Ok, good, that means the UVC driver is doing the right thing here.

But the sysfs files should only be attempted to be removed by the driver
core once, when the device is removed from sysfs, not twice, which is
really odd.

Is there a copy of the "simple app that grabs the device node" anywhere
so that I can test it out here with my USB camera device to try to track
down where the problem is?


Sure. The easiest way is to grab http://git.ideasonboard.org/yavta.git and run

yavta -c /dev/video0

(your mileage may vary if you have other video devices)


I'll point it at the correct device, /dev/video0 is built into this
laptop and can't be physically removed :)


While the application is running, unplug the webcam, and then terminate the
application with ctrl-C.


Ok, will try this out this afternoon and let you know how it goes.


I hate to pester, but wondered if you had found anything obvious.
I really do appreciate you taking the time to look.

Thanks.
  Dave
--
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: Sony imx219 driver?

[Dave Stevenson]

Hi Ramiro

On 13/12/16 13:47, Ramiro Oliveira wrote:

Hi Dave

On 07/21/2016 08:19 PM, Dave Stevenson wrote:

Just a quick query to avoid duplicating effort. Has anyone worked on a
Sony IMX219 (or other Sony sensor) subdevice driver as yet?

With the new Raspberry Pi camera being IMX219, and as Broadcom have
released an soc_camera based driver for the sensor already
(https://android.googlesource.com/kernel/bcm/+/android-bcm-tetra-3.10-lollipop-wear-release/drivers/media/video/imx219.c)
I was going to investigate converting that to a subdevice. I just wanted
to check this wasn't already in someone else's work queue.

A further Google shows that there's also an soc_camera IMX219 driver in
ChromiumOS, copyright Andrew Chew @ Nvidia, but author Guennadi
Liakhovetski who I know posts on here.
https://chromium.googlesource.com/chromiumos/third_party/kernel/+/factory-ryu-6486.14.B-chromeos-3.14/drivers/media/i2c/soc_camera/imx219.c.
The Broadcom one supports 8MPix and 1080P, the Chromium one only 8MP.
Perhaps a hybrid of the feature set? Throw in
https://github.com/ZenfoneArea/android_kernel_asus_zenfone5/blob/master/linux/modules/camera/drivers/media/i2c/imx219/imx219.h
as well, and we have register sets for numerous readout modes, plus
there are the ones in the Pi firmware which can be extracted if necessary.

On a related note, if putting together a system with IMX219 or similar
producing Bayer raw 10, the data on the CSI2 bus is one of the
V4L2_PIX_FMT_SRGGB10P formats. What's the correct way to reflect that
from the sensor subdevice in an MEDIA_BUS_FMT_ enum?
The closest is MEDIA_BUS_FMT_SBGGR10_2X8_PADHI_BE (or LE), but the data
isn't padded (the Pi CSI2 receiver can do the unpacking and padding, but
that just takes up more memory). Or is it MEDIA_BUS_FMT_SBGGR10_1X10
to describe the data on the bus correctly as 10bpp Bayer, and the odd
packing is ignored. Or do we need new enums?


Do you still plan on submitting this driver? If so, do you plan on submitting it
soon?

If you're not planning on submitting it any time soon, do you mind if submit it
myself? I'm planning on having something ready for kernel submission by January.


I've got bogged down in other things and not got anywhere as yet, so go 
for it.
When I've got the CSI2 receiver subdevice working (hopefully soon) then 
I'll happily test your driver with it. For the time being I've been 
working against the TC358743 driver.


  Dave


Thanks,
Ramiro Oliveira
--
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


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

uvcvideo logging kernel warnings on device disconnect

[Dave Stevenson]

Hi All.

I'm working with a USB webcam which has been seen to spontaneously 
disconnect when in use. That's a separate issue, but when it does it 
throws a load of warnings into the kernel log if there is a file handle 
on the device open at the time, even if not streaming.


I've reproduced this with a generic Logitech C270 webcam on:
- Ubuntu 16.04 (kernel 4.4.0-51) vanilla, and with the latest media tree 
from linuxtv.org

- Ubuntu 14.04 (kernel 4.4.0-42) vanilla
- an old 3.10.x tree on an embedded device.

To reproduce:
- connect USB webcam.
- run a simple app that opens /dev/videoX, sleeps for a while, and then 
closes the handle.

- disconnect the webcam whilst the app is running.
- read kernel logs - observe warnings. We get the disconnect logged as 
it occurs, but the warnings all occur when the file descriptor is 
closed. (A copy of the logs from my Ubuntu 14.04 machine are below).


I can fully appreciate that the open file descriptor is holding 
references to a now invalid device, but is there a way to avoid them? Or 
do we really not care and have to put up with the log noise when doing 
such silly things?


Thanks in advance.
  Dave

[157877.297617] usb 1-1: new high-speed USB device number 12 using xhci_hcd
[157877.698744] usb 1-1: New USB device found, idVendor=046d, idProduct=0825
[157877.698747] usb 1-1: New USB device strings: Mfr=0, Product=0, 
SerialNumber=2

[157877.698749] usb 1-1: SerialNumber: E989E680
[157877.699314] uvcvideo: Found UVC 1.00 device  (046d:0825)
[157877.789891] input: UVC Camera (046d:0825) as 
/devices/pci:00/:00:14.0/usb1/1-1/1-1:1.0/input/input688

[157879.135333] usb 1-1: set resolution quirk: cval->res = 384

[157885.686043] usb 1-1: USB disconnect, device number 12

[157901.378104] [ cut here ]
[157901.378111] WARNING: CPU: 2 PID: 21082 at 
/build/linux-lts-xenial-Ev_ZZB/linux-lts-xenial-4.4.0/fs/sysfs/group.c:237 
sysfs_remove_group+0x8d/0x90()

[157901.378113] sysfs group 81ecb6c0 not found for kobject 'event13'
[157901.378114] Modules linked in: snd_usb_audio snd_usbmidi_lib uas 
usb_storage ufs qnx4 hfsplus hfs minix ntfs msdos jfs xfs libcrc32c drbg 
ansi_cprng ctr ccm dm_crypt cmac asus_nb_wmi asus_wmi sparse_keymap 
snd_soc_rt5640 snd_soc_rl6231 snd_soc_core arc4 snd_compress ac97_bus 
snd_pcm_dmaengine uvcvideo snd_seq_midi videobuf2_vmalloc 
videobuf2_memops snd_seq_midi_event videobuf2_v4l2 intel_rapl 
videobuf2_core v4l2_common x86_pkg_temp_thermal videodev 
intel_powerclamp media kvm_intel iwlmvm dm_multipath snd_rawmidi kvm 
mac80211 snd_hda_codec_hdmi irqbypass crct10dif_pclmul crc32_pclmul 
snd_hda_codec_conexant snd_hda_codec_generic btusb rfcomm btrtl btbcm 
bnep snd_hda_intel snd_hda_codec btintel iwlwifi bluetooth snd_seq 
snd_hda_core snd_hwdep aesni_intel aes_x86_64 lrw gf128mul glue_helper 
ablk_helper cryptd input_leds cfg80211 snd_pcm joydev nfsd serio_raw 
mei_me snd_seq_device auth_rpcgss nfs_acl mei processor_thermal_device 
intel_soc_dts_iosf lpc_ich snd_timer binfmt_misc shpchp 
intel_pch_thermal nfs lockd grace sunrpc fscache snd soundcore elan_i2c 
i2c_hid hid int3402_thermal int340x_thermal_zone nls_iso8859_1 dw_dmac 
int3400_thermal snd_soc_sst_acpi dw_dmac_core acpi_als acpi_pad 
i2c_designware_platform kfifo_buf i2c_designware_core 8250_dw 
spi_pxa2xx_platform parport_pc industrialio acpi_thermal_rel ppdev 
coretemp mac_hid lp parport btrfs xor raid6_pq dm_mirror dm_region_hash 
dm_log i915 i2c_algo_bit drm_kms_helper syscopyarea sysfillrect 
sysimgblt fb_sys_fops drm ahci psmouse libahci wmi video sdhci_acpi 
sdhci fjes
[157901.378184] CPU: 2 PID: 21082 Comm: a.out Not tainted 
4.4.0-42-generic #62~14.04.1-Ubuntu

[157901.378185] Hardware name: ASUSTeK COMPUTER INC.
[157901.378186]   8800da3a7bb8 813d51ec 
8800da3a7c00
[157901.378188]  81cdd3b0 8800da3a7bf0 8107d886 

[157901.378190]  81ecb6c0 8800bb2e08c0 8800d93f6248 
8801081b9540

[157901.378192] Call Trace:
[157901.378197]  [] dump_stack+0x63/0x87
[157901.378200]  [] warn_slowpath_common+0x86/0xc0
[157901.378202]  [] warn_slowpath_fmt+0x4c/0x50
[157901.378204]  [] ? kernfs_find_and_get_ns+0x48/0x60
[157901.378206]  [] sysfs_remove_group+0x8d/0x90
[157901.378209]  [] dpm_sysfs_remove+0x57/0x60
[157901.378211]  [] device_del+0x58/0x260
[157901.378213]  [] ? kbd_disconnect+0x22/0x30
[157901.378216]  [] evdev_disconnect+0x23/0x60
[157901.378218]  [] __input_unregister_device+0xb8/0x160
[157901.378219]  [] input_unregister_device+0x47/0x70
[157901.378223]  [] uvc_status_cleanup+0x42/0x50 
[uvcvideo]

[157901.378226]  [] uvc_delete+0x18/0x150 [uvcvideo]
[157901.378228]  [] uvc_release+0x23/0x30 [uvcvideo]
[157901.378233]  [] v4l2_device_release+0xcb/0x100 
[videodev]

[157901.378236]  [] device_release+0x32/0xa0
[157901.378237]  [] kobject_cleanup+0x77/0x190
[157901.378239]  [] kobject_put+0x25/0x50
[157901.378240]  [] put_device+0x17/0x20

Re: Sony imx219 driver?

[Dave Stevenson]

Hi Hans.

On 22/07/16 10:46, Hans Verkuil wrote:



On 07/21/2016 08:19 PM, Dave Stevenson wrote:

Hi All.

Just a quick query to avoid duplicating effort. Has anyone worked on a
Sony IMX219 (or other Sony sensor) subdevice driver as yet?


Not that I am aware of.


OK, glad to hear I won't be duplicating effort.


With the new Raspberry Pi camera being IMX219, and as Broadcom have
released an soc_camera based driver for the sensor already
(https://android.googlesource.com/kernel/bcm/+/android-bcm-tetra-3.10-lollipop-wear-release/drivers/media/video/imx219.c)


#@!@$! Why do they never ever ask us what would be a good example driver to 
use. The
soc-camera framework is deprecated and will likely be gone by the end of the 
year.


Yes, a touch grim.
I'm aware of the project it was done for from my time at Broadcom but 
wasn't directly involved. Partly in their defence:

(a) It was almost 3 years ago.
(b) It's on top of a 3.10 kernel, so lots of the nicer features that are 
now present just weren't there.
(c) However they did bastardise even soc_camera to extract extra info 
from the sensor driver for some of their upper layers. Not so nice.



I was going to investigate converting that to a subdevice. I just wanted
to check this wasn't already in someone else's work queue.

A further Google shows that there's also an soc_camera IMX219 driver in
ChromiumOS, copyright Andrew Chew @ Nvidia, but author Guennadi
Liakhovetski who I know posts on here.
https://chromium.googlesource.com/chromiumos/third_party/kernel/+/factory-ryu-6486.14.B-chromeos-3.14/drivers/media/i2c/soc_camera/imx219.c.


At least for the tegra there is a proper non-soc-camera driver in the works.


non-soc-camera driver for imx219, or for the tegra hardware blocks? I'm 
assuming the latter based on you saying you're not aware of an imx219 
driver being in progress.


As I have mentioned before, the other half of my playing is a subdevice 
driver for the CSI2 receiver on Pi. That bcm android kernel tree also 
released all the info needed for that block into the public domain, so 
I'm not even breaching NDAs in doing so :-) I hope to test my imx219 
driver against that.



The Broadcom one supports 8MPix and 1080P, the Chromium one only 8MP.
Perhaps a hybrid of the feature set?


They usually only implement what they need, so I'd say for chromium they only 
needed 8MP support.


Throw in
https://github.com/ZenfoneArea/android_kernel_asus_zenfone5/blob/master/linux/modules/camera/drivers/media/i2c/imx219/imx219.h
as well, and we have register sets for numerous readout modes, plus
there are the ones in the Pi firmware which can be extracted if necessary.


With all that code it shouldn't be hard to write a proper subdev driver.


Indeed.
I'll try to put something together and submit it to the list. I don't 
know what my timescales will end up being on it though.




On a related note, if putting together a system with IMX219 or similar
producing Bayer raw 10, the data on the CSI2 bus is one of the
V4L2_PIX_FMT_SRGGB10P formats. What's the correct way to reflect that
from the sensor subdevice in an MEDIA_BUS_FMT_ enum?
The closest is MEDIA_BUS_FMT_SBGGR10_2X8_PADHI_BE (or LE), but the data
isn't padded (the Pi CSI2 receiver can do the unpacking and padding, but
that just takes up more memory). Or is it MEDIA_BUS_FMT_SBGGR10_1X10
to describe the data on the bus correctly as 10bpp Bayer, and the odd
packing is ignored. Or do we need new enums?


Just add new enums to media-bus-format.h. It should be clear from comments 
and/or
the naming of the enum what the exact format it, so you'll need to think about
that carefully. Otherwise it's no big deal to add new formats there.


OK, will do.
I wasn't clear from reading the docs how accurately MEDIA_BUS_FMT_ was 
describing the pixel layout as it mentions NOT having a 1:1 mapping with 
V4L2_PIX_FMT_ enums. Having the clarification is great.


Thanks for the advice.
  Dave
--
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

Sony imx219 driver?

[Dave Stevenson]

Hi All.

Just a quick query to avoid duplicating effort. Has anyone worked on a 
Sony IMX219 (or other Sony sensor) subdevice driver as yet?


With the new Raspberry Pi camera being IMX219, and as Broadcom have 
released an soc_camera based driver for the sensor already 
(https://android.googlesource.com/kernel/bcm/+/android-bcm-tetra-3.10-lollipop-wear-release/drivers/media/video/imx219.c) 
I was going to investigate converting that to a subdevice. I just wanted 
to check this wasn't already in someone else's work queue.


A further Google shows that there's also an soc_camera IMX219 driver in 
ChromiumOS, copyright Andrew Chew @ Nvidia, but author Guennadi 
Liakhovetski who I know posts on here. 
https://chromium.googlesource.com/chromiumos/third_party/kernel/+/factory-ryu-6486.14.B-chromeos-3.14/drivers/media/i2c/soc_camera/imx219.c. 
The Broadcom one supports 8MPix and 1080P, the Chromium one only 8MP. 
Perhaps a hybrid of the feature set?
Throw in 
https://github.com/ZenfoneArea/android_kernel_asus_zenfone5/blob/master/linux/modules/camera/drivers/media/i2c/imx219/imx219.h 
as well, and we have register sets for numerous readout modes, plus 
there are the ones in the Pi firmware which can be extracted if necessary.



On a related note, if putting together a system with IMX219 or similar 
producing Bayer raw 10, the data on the CSI2 bus is one of the 
V4L2_PIX_FMT_SRGGB10P formats. What's the correct way to reflect that 
from the sensor subdevice in an MEDIA_BUS_FMT_ enum?
The closest is MEDIA_BUS_FMT_SBGGR10_2X8_PADHI_BE (or LE), but the data 
isn't padded (the Pi CSI2 receiver can do the unpacking and padding, but 
that just takes up more memory). Or is it MEDIA_BUS_FMT_SBGGR10_1X10 
to describe the data on the bus correctly as 10bpp Bayer, and the odd 
packing is ignored. Or do we need new enums?


Thanks.
  Dave
--
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 v2 1/7] v4l: Correct the ordering of LSBs of the 10-bit raw packed formats

[Dave Stevenson]

Hi Hans.

On 20/06/16 18:03, Hans Verkuil wrote:

On 06/20/2016 06:20 PM, Sakari Ailus wrote:

The 10-bit packed raw bayer format documented that the data of the first
pixel of a four-pixel group was found in the first byte and the two
highest bits of the fifth byte. This was not entirely correct. The two
bits in the fifth byte are the two lowest bits. The second pixel occupies
the second byte and third and fourth least significant bits and so on.

Signed-off-by: Sakari Ailus 

As mentioned, this needs confirmation. I wonder, isn't this specified in the UVC
spec?

Regards,

Hans
I'm assuming this is intended to be the same format as generated by many 
Bayer sensors.
Those are defined in both the SMIA CCP2 (section 7.9), and MIPI CSI2 
(section 11.4.4) specs. Whilst nominally restricted, they are both 
available via unofficial websites if you Google for them (I'm happy to 
send links, but didn't want to break mailing list rules by just posting 
them).
CSI2 draft spec Figure 98 "RAW10 Data Transmission on CSI-2 Bus Bitwise 
Illustration" is probably the clearest confirmation of the bit ordering.


dcraw from http://www.cybercom.net/~dcoffin/dcraw/ can consume Raw10 via 
nokia_load_raw

for (dp=data, col=0; col < raw_width; dp+=5, col+=4)
  FORC4 RAW(row,col+c) = (dp[c] << 2) | (dp[4] >> (c << 1) & 3);

And checking against the Raspberry Pi hardware simulator, the RAW10 
parser code has

for (i = 0; i < width; i++) {
   switch ((i + tile_x) & 3) {
  case 0:  val = (buf[0] << 2) | (buf[4] & 3); break;
  case 1:  val = (buf[1] << 2) | ((buf[4] >> 2) & 3); 
break;
  case 2:  val = (buf[2] << 2) | ((buf[4] >> 4) & 3); 
break;

  default: val = (buf[3] << 2) | ((buf[4] >> 6) & 3);


All of those agree with Sakari's update that the first pixel's LSBits 
are in bits 1..0 of byte 5, 2nd pixel in bits 3..2, etc.


Regards,
  Dave
(working on the Pi CSI2 receiver V4L2 driver as there is now sufficient 
data in the public domain to do it. I'll be wanting these formats!)

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