Re: [PATCH RFC] [media] blackfin: add video display driver

[Sylwester Nawrocki <sylvester.nawrocki@gmail.com>]

Hi Scott,

On 04/17/2013 08:57 AM, Scott Jiang wrote:
> Hi Sylwester ,
>
>>> @@ -9,7 +9,18 @@ config VIDEO_BLACKFIN_CAPTURE
>>>            To compile this driver as a module, choose M here: the
>>>            module will be called bfin_capture.
>>>
>>> +config VIDEO_BLACKFIN_DISPLAY
>>> +       tristate "Blackfin Video Display Driver"
>>> +       depends on VIDEO_V4L2&&   BLACKFIN&&   I2C
>>> +       select VIDEOBUF2_DMA_CONTIG
>>> +       help
>>> +         V4L2 bridge driver for Blackfin video display device.
>>
>>
>> Shouldn't it just be "V4L2 output driver", why are you calling it "bridge" ?
>>
> Hmm, capture<->display, input<->output, right?

Yes, input/output from user space POV.

> The kernel docs called it bridge, may "host" sounds better.

I suggested "output" as referring to the "V4L2 output interface" [1].
I guess bridge/host could just be skipped and we could simply put it as:

"V4L2 driver for Blackfin video display (E)PPI interface."

>>> +/*
>>> + * Analog Devices video display driver
>>
>>
>> Sounds a bit too generic.
>>
>>> + *
>>> + * Copyright (c) 2011 Analog Devices Inc.
>>
>>
>> 2011 - 2013 ?
>>
> Written in 2011.

Since you're still actively working on it I would say it makes sense
to put it as 2011 - 2013. At least this is what most people do AFAICS.
But I don't really mind, it's up to you!

>>> +struct disp_fh {
>>> +       struct v4l2_fh fh;
>>> +       /* indicates whether this file handle is doing IO */
>>> +       bool io_allowed;
>>> +};
>>
>>
>> This structure should not be needed when you use the vb2 helpers. Please see
>> below for more details.
>>
> The only question is how the core deal with the permission that which
> file handle can
> stream off the output. I want to impose a rule that only IO handle can stop IO.
> I refer to priority, but current kernel driver export this to user
> space and let user decide it.

As far as I can see there would be no change in behaviour if you used the
helpers. For instance, vidioc_streamon/streamoff ioctls

/* From videobuf2-core.c */

/* The queue is busy if there is a owner and you are not that owner. */
static inline bool vb2_queue_is_busy(struct video_device *vdev, struct 
file *file)
{
	return vdev->queue->owner && vdev->queue->owner != file->private_data;
}

/* vb2 ioctl helpers */

int vb2_ioctl_reqbufs(struct file *file, void *priv,
			  struct v4l2_requestbuffers *p)
{
	struct video_device *vdev = video_devdata(file);
	int res = __verify_memory_type(vdev->queue, p->memory, p->type);

	if (res)
		return res;
	if (vb2_queue_is_busy(vdev, file))
		return -EBUSY;
	res = __reqbufs(vdev->queue, p);
	/* If count == 0, then the owner has released all buffers and he
	   is no longer owner of the queue. Otherwise we have a new owner. */
	if (res == 0)
		vdev->queue->owner = p->count ? file->private_data : NULL;
	return res;
}

int vb2_ioctl_streamon(struct file *file, void *priv, enum v4l2_buf_type i)
{
	struct video_device *vdev = video_devdata(file);

	if (vb2_queue_is_busy(vdev, file))
		return -EBUSY;
	return vb2_streamon(vdev->queue, i);
}

int vb2_ioctl_streamoff(struct file *file, void *priv, enum v4l2_buf_type i)
{
	struct video_device *vdev = video_devdata(file);

	if (vb2_queue_is_busy(vdev, file))
		return -EBUSY;
	return vb2_streamoff(vdev->queue, i);
}

And in your code:


+static int disp_reqbufs(struct file *file, void *priv,
+			struct v4l2_requestbuffers *req_buf)
+{
+	struct disp_device *disp = video_drvdata(file);
+	struct vb2_queue *vq = &disp->buffer_queue;
+	struct v4l2_fh *fh = file->private_data;
+	struct disp_fh *disp_fh = container_of(fh, struct disp_fh, fh);
+
+	if (vb2_is_busy(vq))
+		return -EBUSY;
+
+	disp_fh->io_allowed = true;
+
+	return vb2_reqbufs(vq, req_buf);
+}

+static int disp_streamon(struct file *file, void *priv,
+				enum v4l2_buf_type buf_type)
+{
+	struct disp_device *disp = video_drvdata(file);
+	struct disp_fh *fh = file->private_data;
+	struct ppi_if *ppi = disp->ppi;
+	dma_addr_t addr;
+	int ret;
+
+	if (!fh->io_allowed)
+		return -EBUSY;
+
+	/* call streamon to start streaming in videobuf */
+	ret = vb2_streamon(&disp->buffer_queue, buf_type);
+	if (ret)
+		return ret;
+
	...
+}

+static int disp_streamoff(struct file *file, void *priv,
+				enum v4l2_buf_type buf_type)
+{
+	struct disp_device *disp = video_drvdata(file);
+	struct disp_fh *fh = file->private_data;
+
+	if (!fh->io_allowed)
+		return -EBUSY;
+
+	return vb2_streamoff(&disp->buffer_queue, buf_type);
+}

Please note that you really should be setting io_allowed to true only if
vb2_reqbufs() succeeds.

Hence I wouldn't hesitate to use the core implementation. This way we get
more consistent behaviour across all drivers, which is in line with
what you have currently implemented AFAICT.

[1] http://linuxtv.org/downloads/v4l-dvb-apis/devices.html#output


Thanks,
Sylwester