Re: [PATCH v4 5/7] v4l: Renesas R-Car VSP1 driver

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

Hi Laurent,

On 08/01/2013 12:03 AM, Laurent Pinchart wrote:
> On Wednesday 31 July 2013 23:02:05 Sylwester Nawrocki wrote:
>> On 07/31/2013 05:52 PM, Laurent Pinchart wrote:
>>> The VSP1 is a video processing engine that includes a blender, scalers,
>>> filters and statistics computation. Configurable data path routing logic
>>> allows ordering the internal blocks in a flexible way.
>>>
>>> Due to the configurable nature of the pipeline the driver implements the
>>> media controller API and doesn't use the V4L2 mem-to-mem framework, even
>>> though the device usually operates in memory to memory mode.
>>>
>>> Only the read pixel formatters, up/down scalers, write pixel formatters
>>> and LCDC interface are supported at this stage.
>>>
>>> Signed-off-by: Laurent Pinchart<laurent.pinchart+renesas@ideasonboard.com>
[...]
>>> +static int vsp1_pipeline_stop(struct vsp1_pipeline *pipe)
>>> +{
>>> +	struct vsp1_entity *entity;
>>> +	unsigned long flags;
>>> +	int ret;
>>> +
>>> +	spin_lock_irqsave(&pipe->irqlock, flags);
>>> +	pipe->state = VSP1_PIPELINE_STOPPING;
>>> +	spin_unlock_irqrestore(&pipe->irqlock, flags);
>>> +
>>> +	ret = wait_event_timeout(pipe->wq, pipe->state ==
>>> VSP1_PIPELINE_STOPPED,
>>> +				 msecs_to_jiffies(500));
>>> +	ret = ret == 0 ? -ETIMEDOUT : 0;
>>
>> Wouldn't be -ETIME more appropriate ?
>>
>> #define	ETIME		62	/* Timer expired */
>> ...
>> #define	ETIMEDOUT	110	/* Connection timed out */
>
> $ find Documentation/ -type f -exec egrep -- ETIME[^DO] {} \; | wc
>        7      45     347
> $ find Documentation/ -type f -exec egrep -- ETIMED?OUT {} \; | wc
>       22     135    1162
>
> The only two places where ETIME is used in the Documentation are USB and the
> RxRPC network protocol.
>
> $ find drivers/ -type f -name \*.[ch] -exec grep -- -ETIME[^DO] {} \; | wc
>      295    1037    7339
> $ find drivers/ -type f -name \*.[ch] -exec grep -- -ETIMEDOUT {} \; | wc
>     1156    3769   30590
>
> According to man errno, ETIME seems to be related to XSI STREAMS. I'm fine
> with both, but it seems the kernel is goind towards -ETIMEDOUT.

Indeed, ETIMEDOUT seems to be more widely used. It's a bit strange because
"Connection timed out" looks like a network specific error message and ETIME
("Timer expired") appeared more suitable for cases as above.

I guess it better to stay with ETIMEDOUT then.

>>> +	list_for_each_entry(entity,&pipe->entities, list_pipe) {
>>> +		if (entity->route)
>>> +			vsp1_write(entity->vsp1, entity->route,
>>> +				   VI6_DPR_NODE_UNUSED);
>>> +
>>> +		v4l2_subdev_call(&entity->subdev, video, s_stream, 0);
>>> +	}
>>> +
>>> +	return ret;
>>> +}
>>
>> [...]
>>
>>> +/* ----------------------------------------------------------------------
>>> + * videobuf2 Queue Operations
>>> + */
>>> +
>>> +static int
>>> +vsp1_video_queue_setup(struct vb2_queue *vq, const struct v4l2_format
>>> *fmt,
>>> +		     unsigned int *nbuffers, unsigned int *nplanes,
>>> +		     unsigned int sizes[], void *alloc_ctxs[])
>>> +{
>>> +	struct vsp1_video *video = vb2_get_drv_priv(vq);
>>> +	struct v4l2_pix_format_mplane *format =&video->format;
>>> +	unsigned int i;
>>
>> If you don't support VIDIOC_CREATE_BUFS ioctl then there should probably
>> be at least something like:
>>
>> 	if (fmt)
>> 		return -EINVAL;
>>
>> But it's likely better to add proper handling of 'fmt' right away.
>
> OK, I will do so. What is the driver supposed to do when *fmt isn't supported
> ? Use the closest format as would be returned by try_format() ?

Normally user space should pass valid format, as returned from 
VIDIOC_TRY_FMT
or VIDIOC_G_FMT (this is what V4L2 spec says, as you may already know).

The drivers I wrote just return -EINVAL if an unsupported fourcc is passed.
I'm not sure if this is the behaviour we want in general. I'm inclined to
keep VIDIOC_CREATE_BUFS simple and require user space to pass supported
formats, otherwise the ioctl would fail. Applications anyway have to verify
the format, e.g. with VIDIOC_TRY_FMT.

In any case it would be nice to have the expected behaviour documented in
the videobuf2-core.h header. And perhaps in the VIDIOC_CREATE_BUFS ioctl
DocBook section.

> I suppose this also implies that buffer_prepare() should check whether the
> buffer matches the current format.

Right, buffers unsuitable for the current format should be rejected in
buffer_prepare().

>>> +	*nplanes = format->num_planes;
>>> +
>>> +	for (i = 0; i<   format->num_planes; ++i) {
>>> +		sizes[i] = format->plane_fmt[i].sizeimage;
>>> +		alloc_ctxs[i] = video->alloc_ctx;
>>> +	}
>>> +
>>> +	return 0;
>>> +}
>
> [snip]
>
>>> +static int __vsp1_video_try_format(struct vsp1_video *video,
>>> +				   struct v4l2_pix_format_mplane *pix,
>>> +				   const struct vsp1_format_info **fmtinfo)
>>> +{
>>> +	const struct vsp1_format_info *info;
>>> +	unsigned int width = pix->width;
>>> +	unsigned int height = pix->height;
>>> +	unsigned int i;
>>> +
>>> +	/* Retrieve format information and select the default format if the
>>> +	 * requested format isn't supported.
>>> +	 */
>>
>> Nitpicking: Isn't proper multi-line comment style
>>
>> 	/*
>> 	 * Retrieve format information and select the default format if the
>> 	 * requested format isn't supported.
>> 	 */
>>
>> ?
>
> Yes it is. I got used to the
>
> /* foo
>   * bar
>   */
>
> style as it's more compact.
>
>> In fact the media subsystem code is pretty messy WRT that detail.
>
> Documentation/CodingStyle mentions
>
> The preferred style for long (multi-line) comments is:
>
>          /*
>           * This is the preferred style for multi-line
>           * comments in the Linux kernel source code.
>           * Please use it consistently.
>           *
>           * Description:  A column of asterisks on the left side,
>           * with beginning and ending almost-blank lines.
>           */
>
> For files in net/ and drivers/net/ the preferred style for long (multi-line)
> comments is a little different.
>
>          /* The preferred comment style for files in net/ and drivers/net
>           * looks like this.
>           *
>           * It is nearly the same as the generally preferred comment style,
>           * but there is no initial almost-blank line.
>           */
>
> I'd love to add drivers/media/ to that list ;-)

Yup, that's one of the options ;) I personally don't mind which variant
is used, as long as it is only one of them and used consistently.

But unfortunately it looks like it's to late already and nobody is going
to bother with patches that change comments to one style or the other.
I guess I wanted mostly to bring some attention to the problem rather
than raising objections to this particular patch.

--
Regards,
Sylwester