Hi Robert,

On 07/31/2016 05:03 PM, Robert Jarzmik wrote:

Hi Hans,


Hans Verkuil [off-list ref] writes:

quoted

On 04/02/2016 04:26 PM, Robert Jarzmik wrote:

quoted

diff --git a/drivers/media/platform/soc_camera/pxa_camera.c b/drivers/media/platform/soc_camera/pxa_camera.c
index b8dd878e98d6..30d266bbab55 100644
--- a/drivers/media/platform/soc_camera/pxa_camera.c
+++ b/drivers/media/platform/soc_camera/pxa_camera.c

When you prepare the final patch series, please put the driver in
drivers/media/platform/pxa-camera and not in the soc-camera directory.

Sure.
Would you accept the final patch to make the move, so that I keep the
bisectability, ie. that all patches are applied while still in ../soc_camera,
and then the final one making just the move to .../platform ?

Sure, that's fine.

quoted

quoted

+	if (format->name)
+		strlcpy(f->description, format->name, sizeof(f->description));

You can drop this since the core fills in the description. That means the
'name' field of struct soc_mbus_pixelfmt is not needed either.

Ok, let's try this, for v3.

quoted

quoted

+static int pxac_vidioc_querycap(struct file *file, void *priv,
+				struct v4l2_capability *cap)
+{
+	strlcpy(cap->bus_info, "platform:pxa-camera", sizeof(cap->bus_info));
+	strlcpy(cap->driver, PXA_CAM_DRV_NAME, sizeof(cap->driver));
+	strlcpy(cap->card, pxa_cam_driver_description, sizeof(cap->card));
+	cap->device_caps = V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_STREAMING;
+	cap->capabilities = cap->device_caps | V4L2_CAP_DEVICE_CAPS;

Tiny fix: you can drop the capabilities assignment: the v4l2 core does that
for you these days.

Well, above kernel v4.7, if I drop the assignement, v4l2-compliance -s finds 2
new errors:
	Required ioctls:
			fail: v4l2-compliance.cpp(534): dcaps & ~caps
		test VIDIOC_QUERYCAP: FAIL
	
	Allow for multiple opens:
		test second video open: OK
			fail: v4l2-compliance.cpp(534): dcaps & ~caps
		test VIDIOC_QUERYCAP: FAIL
		test VIDIOC_G/S_PRIORITY: OK

So there is something fishy here if the core provides this data ...

Sorry, I forgot to say that you have to set the device_caps field of struct
video_device to V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_STREAMING. You do that
where you set up the other fields of that structure.

The advantage is that the v4l2 core now knows the device capabilities and
it can use that information in the future.

quoted

quoted

+static int pxac_vidioc_enum_input(struct file *file, void *priv,
+				  struct v4l2_input *i)
+{
+	if (i->index > 0)
+		return -EINVAL;
+
+	memset(i, 0, sizeof(*i));

The memset can be dropped, it's cleared for you.

OK, for v3.

quoted

quoted

+static void pxac_vb2_queue(struct vb2_buffer *vb)
+{
+	struct pxa_buffer *buf = vb2_to_pxa_buffer(vb);
+	struct pxa_camera_dev *pcdev = vb2_get_drv_priv(vb->vb2_queue);
+
+	dev_dbg(pcdev_to_dev(pcdev),
+		 "%s(vb=%p) nb_channels=%d size=%lu active=%p\n",
+		__func__, vb, pcdev->channels, vb2_get_plane_payload(vb, 0),
+		pcdev->active);
+
+	list_add_tail(&buf->queue, &pcdev->capture);
+
+	pxa_dma_add_tail_buf(pcdev, buf);
+
+	if (!pcdev->active)
+		pxa_camera_start_capture(pcdev);

This is normally done from start_streaming. Are you really sure this is the right
place? I strongly recommend moving this start_capture call.

Well, it was at least with the previous framework.
Previously this was done here to "hot-queue" a buffer :
 - if a previous capture was still running, the buffer was queued by
   pxa_dma_add_tail_buf(), and no restart of the DMA pump was necessary
 - if the previous capture was finished, a new one was initiated

Now if the new framework takes care of that, I can move the
pxa_camera_start_capture() into start_streaming(), no problem, let me try in the
next patchset. That might take a bit of time because testing both the
"hot-queue" and the "queue but hot-queuing missed" is a bit tricky.

quoted

You may also want to use the struct vb2queue min_buffers_needed field to specify
the minimum number of buffers that should be queued up before start_streaming can
be called. Whether that's needed depends on your DMA engine.

I have no minimum required by the pxa dmaengine driver, that's good.

quoted

quoted

+
+	v4l2_disable_ioctl(vdev, VIDIOC_G_STD);
+	v4l2_disable_ioctl(vdev, VIDIOC_S_STD);
+	v4l2_disable_ioctl(vdev, VIDIOC_ENUMSTD);

I don't think this is needed since the tvnorms field of struct video_device == 0,
signalling that there is no STD support.

OK, for v3.

Cheers.

Regards,

	Hans