Hello
On Tue, Jul 14, 2015 at 11:32 AM, Laurent Pinchart
<laurent.pinch...@ideasonboard.com> wrote:
> Hi Hans,
>
> On Tuesday 14 July 2015 16:19:50 Hans Verkuil wrote:
>> On 07/13/15 10:03, Sakari Ailus wrote:
>> > Helen Fornazier wrote:
>> >> On Tue, Jun 30, 2015 at 4:26 PM, Helen Fornazier wrote:
>> >>> On Tue, Jun 30, 2015 at 6:19 AM, Sakari Ailus wrote:
>> >>>> Laurent Pinchart wrote:
>> >>>>> On Monday 29 June 2015 10:23:34 Sakari Ailus wrote:
>> >>>>>> Helen Fornazier wrote:
>> >>>>>>> According to the V4L2 API, the VIDIOC_STREAMON ioctl should return
>> >>>
>> >>> EPIPE
>> >>>
>> >>>>>>> when the pipeline configuration is invalid.
>> >>>>>>>
>> >>>>>>> As the .vidioc_streamon in the v4l2_ioctl_ops usually forwards the
>> >>>
>> >>> error
>> >>>
>> >>>>>>> caused by the v4l2_subdev_link_validate_default (if it is in use),
>> >>>>>>> it
>> >>>>>>> should return -EPIPE if it detects a format mismatch in the pipeline
>> >>>>>>> configuration
>> >>>>>>
>> >>>>>> Only link configuration errors have yielded -EPIPE so far, sub-device
>> >>>>>> format configuration error has returned -INVAL instead as you
>> >>>>>> noticed.
>> >>>>>
>> >>>>> It should also be noted that while v4l2_subdev_link_validate() will
>> >>>
>> >>> return -
>> >>>
>> >>>>> EINVAL in case of error, the only driver that performs custom link
>> >>>
>> >>> validation
>> >>>
>> >>>>> (omap3isp/ispccdc.c) will return -EPIPE.
>> >>>>
>> >>>> Good point. That has escaped me until now.
>> >>>>
>> >>>>>> There are not many sources of -EINVAL while enabling streaming and
>> >>>>>> all others are directly caused by the application; I lean towards
>> >>>>>> thinking the code is good as it was. The documentation could be
>> >>>>>> improved though. It may not be clear which error codes could be
>> >>>>>> caused by different conditions.
>> >>>>>>
>> >>>>>> The debug level messages from media module
>> >>>>>> (drivers/media/media-entity.c) do provide more information if needed,
>> >>>>>> albeit this certainly is not an application interface.
>> >>>>>>
>> >>>>>> I wonder what others think.
>> >>>>>
>> >>>>> There's a discrepancy between the implementation and the
>> >>>>> documentation, so at least one of them need to be fixed. -EPIPE would
>> >>>>> be coherent with the documentation and seems appropriately named, but
>> >>>>> another error code would allow userspace to tell link configuration
>> >>>>> and format configuration problems apart.
>> >>>>
>> >>>> That was the original intent, I think.
>> >>>>
>> >>>>> Do you think -EINVAL is the most appropriate error code for format
>> >>>>> configuration ? It's already used to indicate that the stream type is
>> >>>>> invalid or that not enough buffers have been allocated, and is also
>> >>>>> used by drivers directly for various purposes.
>> >>>>
>> >>>> That's true, it's been used also for that purpose. At the time this
>> >>>> certainly was not the primary concern. If you can think of a better
>> >>>> error code for the purpose (than EINVAL) I'm certainly fine with using
>> >>>> one. I still think that -EPIPE is worse for telling about incorrect
>> >>>> format configuration than -EINVAL since it's relatively easy to avoid -
>> >>>> EINVAL for the documented reasons.
>> >>>
>> >>> I'd like just to point out where in the docs EPIPE for format mismatch
>> >>> is specified, as it is not described in the streamon page as I thought
>> >>> it would, but it is in the subdev page in case anyone is looking for
>> >>> it (as I took some time to find it too):
>> >>>
>> >>> http://linuxtv.org/downloads/v4l-dvb-apis/subdev.html
>> >>> "Applications are responsible for configuring coherent parameters on
>> >>> the whole pipeline and making sure that connected pads have compatible
>> >>> formats. The pipeline is checked for formats mismatch at
>> >>> VIDIOC_STREAMON time, and an EPIPE error code is then returned if the
>> >>> configuration is invalid"
>> >>>
>> >>> So maybe the doc should be improved as you already stated.
>> >>
>> >> I would like to revive this subject.
>> >>
>> >> Should we change the docs? Change the -EINVAL to -EPIPE, or create
>> >> another error code? What are your opinion?
>> >>
>> >> I read in the docs of dev-kmsg that EPIPE is returned when messages get
>> >> overwritten, and in other parts of the code EPIPE is returned when there
>> >> is an error in the pipeline communication level while trying to send
>> >> information through the pipe or a pipe broken error.
>> >>
>> >> But in the error-codes.txt files, the EPIPE error is defined as:
>> >> *EPIPE "The pipe type specified in the URB doesn't match the endpoint's
>> >> actual type"*
>> >
>> > This exact definition sound USB specific to me.
>> >
>> >> Then, if EPIPE is used when types don't match between two endpoints, it
>> >> seems reasonable to me to use EPIPE when formats don't match either. Or
>> >> do "types" in this context have a specific definition? I don't know much
>> >> about URB, you may be able to judge this better.
>> >
>> > A short recap of the current situation as far as I understand it:
>> >
>> > - MC link validation failure yields EPIPE to the user space,
>> >
>> > - V4L2 sub-device format validation failure generally results in EINVAL,
>> > except that
>>
>> I think that returning EINVAL here is wrong. EINVAL is returned when you
>> set e.g. a format and the format is invalid. In this case the format for
>> each subdev is perfectly fine, it's just that the sink and source formats
>> do not match.
>>
>> Rather than returning EINVAL I think ENOLINK would work well here as you
>> can't setup a link between two entities. And EPIPE can still be used
>> for other higher-level pipeline errors.
>>
>> > - omap3isp CCDC driver returns EPIPE instead and
>>
>> That seems definitely the wrong thing to do.
>
> The VIDIOC_STREAMON documentation states that
>
> "EPIPE
>
> The driver implements pad-level format configuration and the pipeline
> configuration is invalid."
>
> According to the documentation, EINVAL is clearly wrong, and EPIPE should be
> used. I'm open to the idea of using different error codes to indicate severed
> link errors and links with an invalid configuration, but how about backward
> compatibility ?
>
I don't think backward compatibility is a big issue, as it seems the
code wasn't returning what the docs says and nobody complained about
it so far.
I see 4 options then:
1) Keep -EINVAL and update the docs
2) Change the code to -EPIPE (there is no need in updating the docs here)
3) Change the code to -ENOLINK and update the docs
4) Create another error (-EFMT for example), change the code to it and
update the docs
which one do you prefer?
>> > - EINVAL is used for many other purposes.
>> >
>> > The issues are inconsistency between omap3isp CCDC and other drivers in
>> > informing the user the sub-device format configuration is wrong. Also
>> > V4L2 sub-device format validation error cannot be told apart from other
>> > errors. These problems should be fixed, so that all three sources of
>> > errors yield a different error code (MC link validation, V4L2 format
>> > configuration and other plain V4L2 related errors).
>> >
>> > V4L2 will continue using EINVAL, that's for sure.
>> >
>> > Another error code I could think of is EMLINK ("Too many links"), which
>> > is not a perfect match, but could be used. This is a better match for a
>> > link validation failure; V4L2 sub-device link validation failure would
>> > then use EPIPE (as omap3isp CCDC driver already does).
>> >
>> > Another option could be that V4L2 format validation failure would use
>> > ENOEXEC ("Exec format error") instead, and EPIPE would be left to link
>> > validation failures.
>> >
>> > Better suggestions are welcome of course. I think I'm leaning towards
>> > the first option, but from backwards compatibility point of view the
>> > latter is better. The MC is no longer experimental so the latter might
>> > be the only option.
>> >
>> > My view is that this boils down to picking the most suitable error
>> > codes. Then fixing the documentation is easy.
>> >
>> > I wonder what Laurent and Hans think.
>
> --
> Regards,
>
> Laurent Pinchart
>
--
Helen Fornazier
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
