Re: [PATCHv3] [media] rcar-vin: add Renesas R-Car VIN driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Niklas,

Some quick review comments...

On 04/02/2016 10:52 AM, Niklas Söderlund wrote:
A V4L2 driver for Renesas R-Car VIN driver that do not depend on
soc_camera. The driver is heavily based on its predecessor and aims to
replace it.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@xxxxxxxxxxxx>
---

The driver is tested on Koelsch and can do streaming using qv4l2 and
grab frames using yavta. It passes a v4l2-compliance (git master) run
without any failures, see bellow for output. Some issues I know about
but will have to wait for future work in other patches.
  - One can not bind/unbind the subdevice and continue using the driver.
  - Do not support FIELD_ALTERNATE.
  - Suggested compat string "renesas,rcar-gen2-vin" is not included. Will
    address this in a separate patch together with gen3.

The goal is to replace the soc_camera driver completely to prepare for
Gen3 enablement. I have therefor chosen to inherit the
CONFIG_VIDEO_RCAR_VIN name for this new driver and renamed the
soc_camera driver CONFIG_VIDEO_RCAR_VIN_OLD.

* Changes since v2
- Fix review comments from Hans Verkuil, thanks!
     - Update description in Kconfig
     - Drop V4L2_SEL_TGT_COMPOSE_PADDED
     - Wrong size for NV16 image
     - Copy ycbcr_enc and xfer_func when keeping old format.
     - Add vidioc_cropcap
     - Return -ENOBUFS in start_streaming to signal more buffers are
       needed instead of sleeping in a critical section...
     - Move all v4l2 ioctls and file ops to rcar-v4l2.c (and as a follow
       up moved all HW functions to rcar-dma.c to increase readability).
- Fixed RGB formats 's/V4L2_PIX_FMT_RGB555X/V4L2_PIX_FMT_XRGB555' and
   's/V4L2_PIX_FMT_RGB32/V4L2_PIX_FMT_XBGR32'. This was an error carried
   over from soc-camera dirver, whit this fix I get correct colors in
   qv4l2.
- Rework how media bus type and flags are handled. Instead of defining
   own values and a unsigned int use struct v4l2_mbus_config to store the
   configuration parsed from DT.
- Remove duplicated code from the v4l2_file_operations release code
   path. There is no need to try and stop the streaming from here. If
   start_streaming have been called stop_streaming will be called by the
   framework stopping the streaming.
- Remove all special checks for the chip RCAR_E1. There are no compat
   string that will select this chip model. Neither for this driver or
   its predecessor in soc-camera.
- Force an width alignment of 32 if the NV16 format is used due to HW
   limitation.

* Changes since RFC/PATCH
- Fixed review comments from Hans Verkuil, thanks for reviewing.
- Added vidioc_[gs]_selection crop and composition is supported. Thanks
   Laurent for taking the time and explaining to me how to do
   composition.
- Reworked the DMA flow to better support single and continues frame
   grabbing mode.
- Dropped a lot of the formats that was ported from soc_camera, once I
   looked at it in a working driver it was obvious that the rcar_vin
   soc_camera driver did not support them.
- Added better comments for the core structs
- Fixed copyright in file headers
- A lot more testing.

I have made a few small additions to the adv7180.c driver while doing
this driver but are posted in a separate patch. For completeness I
included the output of v4l2-compliance both with and with out the
adv7180 enhancements. The adv7180 additions enables the std and tvnorms
code paths so it tests a bit more of this driver.

There is a failure reported here but it's a false positive and is
addressed in Hans Verkuil series '[PATCHv2 0/2] v4l2-ioctl: cropcap
improvement'. If I apply that series to my tree the failure goes away.


<snip>

diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
new file mode 100644
index 0000000..85a1be7
--- /dev/null
+++ b/drivers/media/platform/rcar-vin/rcar-dma.c

<snip>

+static bool rvin_fill_hw(struct rvin_dev *vin)
+{
+	int slot, limit;
+
+	limit = vin->continuous ? HW_BUFFER_NUM : 1;
+
+	for (slot = 0; slot < limit; slot++)
+		if (!rvin_fill_hw_slot(vin, slot))
+			return false;
+	return true;
+}
+
+static irqreturn_t rvin_irq(int irq, void *data)
+{
+	struct rvin_dev *vin = data;
+	u32 int_status;
+	int slot;
+	unsigned int sequence, handled = 0;
+	unsigned long flags;
+
+	spin_lock_irqsave(&vin->qlock, flags);
+
+	int_status = rvin_get_interrupt_status(vin);
+	if (!int_status)
+		goto done;
+
+	rvin_ack_interrupt(vin);
+	handled = 1;
+
+	/* Nothing to do if capture status is 'STOPPED' */
+	if (vin->state == STOPPED) {
+		vin_dbg(vin, "IRQ state stopped\n");
+		goto done;
+	}
+
+	/* Wait for HW to shutdown */
+	if (vin->state == STOPPING) {
+		if (!rvin_capture_active(vin)) {
+			vin_dbg(vin, "IRQ hw stopped and we are stopping\n");
+			complete(&vin->capture_stop);
+		}
+		goto done;
+	}
+
+	/* Prepare for capture and update state */
+	slot = rvin_get_active_slot(vin);
+	sequence = vin->sequence++;
+
+	vin_dbg(vin, "IRQ %02d: %d\tbuf0: %c buf1: %c buf2: %c\tmore: %d\n",
+		sequence, slot,
+		slot == 0 ? 'x' : vin->queue_buf[0] != NULL ? '1' : '0',
+		slot == 1 ? 'x' : vin->queue_buf[1] != NULL ? '1' : '0',
+		slot == 2 ? 'x' : vin->queue_buf[2] != NULL ? '1' : '0',
+		!list_empty(&vin->buf_list));
+
+	/* HW have written to a slot that is not prepared we are in trouble */
+	if (WARN_ON((vin->queue_buf[slot] == NULL)))
+		goto done;
+
+	/* Capture frame */
+	vin->queue_buf[slot]->field = vin->format.field;
+	vin->queue_buf[slot]->sequence = sequence;
+	vin->queue_buf[slot]->vb2_buf.timestamp = ktime_get_ns();
+	vb2_buffer_done(&vin->queue_buf[slot]->vb2_buf, VB2_BUF_STATE_DONE);
+	vin->queue_buf[slot] = NULL;
+
+	/* Prepare for next frame */
+	if (!rvin_fill_hw(vin)) {
+
+		/*
+		 * Can't supply HW with new buffers fast enough. Halt
+		 * capture until more buffers are available.
+		 */
+		vin->state = STALLED;
+
+		/*
+		 * The continuous capturing requires an explicit stop
+		 * operation when there is no buffer to be set into
+		 * the VnMBm registers.
+		 */
+		if (vin->continuous) {
+			rvin_capture_off(vin);
+			vin_dbg(vin, "IRQ %02d: hw not ready stop\n", sequence);
+		}
+	} else {
+		/*
+		 * The single capturing requires an explicit capture
+		 * operation to fetch the next frame.
+		 */
+		if (!vin->continuous)
+			rvin_capture_on(vin);
+	}
+done:
+	spin_unlock_irqrestore(&vin->qlock, flags);
+
+	return IRQ_RETVAL(handled);
+}
+
+/* Need to hold qlock before calling */
+static void return_all_buffers(struct rvin_dev *vin,
+			       enum vb2_buffer_state state)
+{
+	struct rvin_buffer *buf, *node;
+
+	list_for_each_entry_safe(buf, node, &vin->buf_list, list) {
+		vb2_buffer_done(&buf->vb.vb2_buf, state);
+		list_del(&buf->list);
+	}
+}
+
+static int rvin_queue_setup(struct vb2_queue *vq, unsigned int *nbuffers,
+			    unsigned int *nplanes, unsigned int sizes[],
+			    void *alloc_ctxs[])
+
+{
+	struct rvin_dev *vin = vb2_get_drv_priv(vq);
+
+	alloc_ctxs[0] = vin->alloc_ctx;
+	/* Make sure the image size is large enough. */
+	if (*nplanes)
+		return sizes[0] < vin->format.sizeimage ? -EINVAL : 0;
+
+	*nplanes = 1;
+	sizes[0] = vin->format.sizeimage;
+
+	return 0;
+};
+
+static int rvin_buffer_prepare(struct vb2_buffer *vb)
+{
+	struct rvin_dev *vin = vb2_get_drv_priv(vb->vb2_queue);
+	unsigned long size = vin->format.sizeimage;
+
+	if (vb2_plane_size(vb, 0) < size) {
+		vin_err(vin, "buffer too small (%lu < %lu)\n",
+			vb2_plane_size(vb, 0), size);
+		return -EINVAL;
+	}
+
+	vb2_set_plane_payload(vb, 0, size);
+
+	return 0;
+}
+
+static void rvin_buffer_queue(struct vb2_buffer *vb)
+{
+	struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
+	struct rvin_dev *vin = vb2_get_drv_priv(vb->vb2_queue);
+	unsigned long flags;
+
+	spin_lock_irqsave(&vin->qlock, flags);
+
+	list_add_tail(to_buf_list(vbuf), &vin->buf_list);
+
+	/*
+	 * If capture is stalled add buffer to HW and restart
+	 * capturing if HW is ready to continue.
+	 */
+	if (vin->state == STALLED)
+		if (rvin_fill_hw(vin))
+			rvin_capture_on(vin);
+
+	spin_unlock_irqrestore(&vin->qlock, flags);
+}
+
+static int rvin_start_streaming(struct vb2_queue *vq, unsigned int count)
+{
+	struct rvin_dev *vin = vb2_get_drv_priv(vq);
+	struct v4l2_subdev *sd;
+	unsigned long flags;
+	int ret;
+
+	sd = vin_to_sd(vin);
+	v4l2_subdev_call(sd, video, s_stream, 1);
+
+	spin_lock_irqsave(&vin->qlock, flags);
+
+	vin->state = RUNNING;
+	vin->sequence = 0;
+	init_completion(&vin->capture_stop);
+
+	/* Continuous capture requires more buffers then there is HW slots */
+	vin->continuous = count > HW_BUFFER_NUM;
+
+	/*
+	 * Wait for more buffers if HW is not ready to start capturing. By
+	 * returning -ENOBUFS the vb2 framework will call start_streaming
+	 * again once one more buffer is queued.
+	 */
+	if (!rvin_fill_hw(vin)) {
+		vin_dbg(vin, "HW not ready to start, wait for more buffers\n");
+		ret = -ENOBUFS;

Normally setting the min_buffers_needed field is enough to ensure this (i.e. start_streaming
won't be called until 'min_buffers_needed' buffers have been queued).

So why isn't that enough in this case? This should probably be in a comment here, since
it is not obvious (at least not to me).

I am not actually sure returning ENOBUFS works at all. Looking at the vb2_core_qbuf()
code I see that it does seem to queue the buffer, but if start_streaming returns an
error it will pass that on, so VIDIOC_QBUF returns an error, but it still queued the buffer,
which is unexpected from a userspace perspective. I wonder if this isn't a vb2 bug.

I do know that userspace wouldn't know what to do with a -ENOBUFS error.

If support for -ENOBUFS is needed, then some vb2 work would be needed to make this
work (vb2_core_qbuf should check for -ENOBUFS and just return 0 to userspace in that
case).

+		goto out;
+	}
+
+	ret = rvin_capture_start(vin);
+out:
+	spin_unlock_irqrestore(&vin->qlock, flags);
+

If case of failure start_streaming should return all buffers with state
VB2_BUF_STATE_QUEUED to ensure proper operation.

+	return ret;
+}

<snip>

+static int __rvin_dma_try_format(struct rvin_dev *vin,
+				 u32 which,
+				 struct v4l2_pix_format *pix,
+				 struct rvin_sensor *sensor)
+{
+	const struct rvin_video_format *info;
+	u32 rwidth, rheight, walign;
+
+	/* Requested */
+	rwidth = pix->width;
+	rheight = pix->height;
+
+	/*
+	 * Retrieve format information and select the current format if the
+	 * requested format isn't supported.
+	 */
+	info = rvin_format_from_pixel(pix->pixelformat);
+	if (!info) {
+		vin_dbg(vin, "Format %x not found, keeping %x\n",
+			pix->pixelformat, vin->format.pixelformat);
+		pix->pixelformat = vin->format.pixelformat;
+		pix->colorspace = vin->format.colorspace;
+		pix->field = vin->format.field;
+		pix->ycbcr_enc = vin->format.ycbcr_enc;
+		pix->xfer_func = vin->format.xfer_func;
+		pix->quantization = vin->format.quantization;

Isn't it easier to do:

		pix = vin->format;
		pix->width = rwidth;
		pix->height = rheight;

It's more robust when new fields are added to the pix_format struct.

+	}
+
+	/* Always recalculate */
+	pix->bytesperline = 0;
+	pix->sizeimage = 0;
+
+	/* Limit to sensor capabilities */
+	__rvin_dma_try_format_sensor(vin, which, pix, sensor);
+
+	/* If sensor can't match format try if VIN can scale */
+	if (sensor->width != rwidth || sensor->height != rheight)
+		rvin_scale_try(vin, pix, rwidth, rheight);
+
+	/* HW limit width to a multiple of 32 (2^5) for NV16 else 2 (2^1) */
+	walign = vin->format.pixelformat == V4L2_PIX_FMT_NV16 ? 5 : 1;
+
+	/* Limit to VIN capabilities */
+	v4l_bound_align_image(&pix->width, 2, RVIN_MAX_WIDTH, walign,
+			      &pix->height, 4, RVIN_MAX_HEIGHT, 2, 0);
+
+	switch (pix->field) {
+	case V4L2_FIELD_NONE:
+	case V4L2_FIELD_TOP:
+	case V4L2_FIELD_BOTTOM:
+	case V4L2_FIELD_INTERLACED_TB:
+	case V4L2_FIELD_INTERLACED_BT:
+	case V4L2_FIELD_INTERLACED:
+		break;
+	default:
+		pix->field = V4L2_FIELD_NONE;
+		break;
+	}
+
+	pix->bytesperline = max_t(u32, pix->bytesperline,
+				  rvin_format_bytesperline(pix));
+	pix->sizeimage = max_t(u32, pix->sizeimage,
+			       rvin_format_sizeimage(pix));
+
+	vin_dbg(vin, "Requested %ux%u Got %ux%u bpl: %d size: %d\n",
+		rwidth, rheight, pix->width, pix->height,
+		pix->bytesperline, pix->sizeimage);
+
+	return 0;
+}
+
+static int rvin_querycap(struct file *file, void *priv,
+			 struct v4l2_capability *cap)
+{
+	struct rvin_dev *vin = video_drvdata(file);
+
+	strlcpy(cap->driver, KBUILD_MODNAME, sizeof(cap->driver));
+	strlcpy(cap->card, "R_Car_VIN", sizeof(cap->card));
+	snprintf(cap->bus_info, sizeof(cap->bus_info), "platform:%s",
+		 dev_name(vin->dev));
+	cap->device_caps = V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_STREAMING |
+		V4L2_CAP_READWRITE;
+	cap->capabilities = cap->device_caps | V4L2_CAP_DEVICE_CAPS;
+	return 0;
+}
+
+static int rvin_try_fmt_vid_cap(struct file *file, void *priv,
+				struct v4l2_format *f)
+{
+	struct rvin_dev *vin = video_drvdata(file);
+	struct rvin_sensor sensor;
+
+	return __rvin_dma_try_format(vin, V4L2_SUBDEV_FORMAT_TRY, &f->fmt.pix,
+				     &sensor);
+}
+
+static int rvin_s_fmt_vid_cap(struct file *file, void *priv,
+			      struct v4l2_format *f)
+{
+	struct rvin_dev *vin = video_drvdata(file);
+	struct rvin_sensor sensor;
+	int ret;
+
+	if (vb2_is_busy(&vin->queue))
+		return -EBUSY;
+
+	ret = __rvin_dma_try_format(vin, V4L2_SUBDEV_FORMAT_ACTIVE, &f->fmt.pix,
+				    &sensor);
+	if (ret)
+		return ret;
+
+	vin->sensor.width = sensor.width;
+	vin->sensor.height = sensor.height;
+
+	vin->format = f->fmt.pix;
+
+	return 0;
+}
+
+static int rvin_g_fmt_vid_cap(struct file *file, void *priv,
+			      struct v4l2_format *f)
+{
+	struct rvin_dev *vin = video_drvdata(file);
+
+	f->fmt.pix = vin->format;
+
+	return 0;
+}
+
+static int rvin_enum_fmt_vid_cap(struct file *file, void *priv,
+				 struct v4l2_fmtdesc *f)
+{
+	if (f->index >= ARRAY_SIZE(rvin_formats))
+		return -EINVAL;
+
+	f->pixelformat = rvin_formats[f->index].fourcc;
+
+	return 0;
+}
+
+static int rvin_g_selection(struct file *file, void *fh,
+			    struct v4l2_selection *s)
+{
+	struct rvin_dev *vin = video_drvdata(file);
+
+	switch (s->target) {
+	case V4L2_SEL_TGT_CROP_BOUNDS:
+	case V4L2_SEL_TGT_CROP_DEFAULT:
+		s->r.left = s->r.top = 0;
+		s->r.width = vin->sensor.width;
+		s->r.height = vin->sensor.height;
+		break;
+	case V4L2_SEL_TGT_CROP:
+		s->r = vin->crop;
+		break;
+	case V4L2_SEL_TGT_COMPOSE_BOUNDS:
+	case V4L2_SEL_TGT_COMPOSE_DEFAULT:
+		s->r.left = s->r.top = 0;
+		s->r.width = vin->format.width;
+		s->r.height = vin->format.height;
+		break;
+	case V4L2_SEL_TGT_COMPOSE:
+		s->r = vin->compose;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static void rect_set_min_size(struct v4l2_rect *r,
+			      const struct v4l2_rect *min_size)
+{
+	if (r->width < min_size->width)
+		r->width = min_size->width;
+	if (r->height < min_size->height)
+		r->height = min_size->height;
+}
+
+static void rect_set_max_size(struct v4l2_rect *r,
+			      const struct v4l2_rect *max_size)
+{
+	if (r->width > max_size->width)
+		r->width = max_size->width;
+	if (r->height > max_size->height)
+		r->height = max_size->height;
+}
+
+static void rect_map_inside(struct v4l2_rect *r,
+			    const struct v4l2_rect *boundary)
+{
+	rect_set_max_size(r, boundary);
+
+	if (r->left < boundary->left)
+		r->left = boundary->left;
+	if (r->top < boundary->top)
+		r->top = boundary->top;
+	if (r->left + r->width > boundary->width)
+		r->left = boundary->width - r->width;
+	if (r->top + r->height > boundary->height)
+		r->top = boundary->height - r->height;
+}

I think it is time we turn this into common code. I'll post a patch for that.

Regards,

	Hans



[Index of Archives]     [Linux Samsung SOC]     [Linux Wireless]     [Linux Kernel]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Device Mapper]

  Powered by Linux