Hi Sylwester,
On Friday 02 August 2013 00:31:15 Sylwester Nawrocki wrote:
> 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@xxxxxxxxxxxxxxxx>
>
> [...]
>
> >>> +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.
OK. It's easier as well, no change needed :-)
> >>> + 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.
I agree with that, applications should pass a valid format to
VIDIOC_CREATE_BUFS. I will thus return -EINVAL if any of the format fields is
not valid (most likely by running the format through TRY_FMT and check if
anything changes).
> 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.
That's a good idea. I'll submit patches.
> > 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().
OK.
> >>> + *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.
I'll try to keep an eye on that. I'll probably keep using my comments style
for drivers I write, but will adjust to the format in use when submitting
patches for existing code.
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
