Hi Sakari, On Thursday 13 August 2015 15:23:26 Sakari Ailus wrote: > On Mon, Aug 10, 2015 at 04:07:30PM +0200, Hans Verkuil wrote: > > On 07/14/2015 04:32 PM, Laurent Pinchart wrote: > >> 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. > > > > I think I was ambiguous here. What is wrong here is that it replaces the > > actual error code with EPIPE instead of passing it along to userspace. > > I'm not sure if I follow you. ccdc_link_validate() will not obtain the error > code from anywhere else; instead, it returns -EPIPE if the media bus > formats at both ends of the link do not match. > > >> 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 ? > > > > Applications should *always* check for any error. An application that only > > checks for a specific error and assumes that any other error is the same > > as 'Success' is obviously broken. I have no problem with adding a separate > > error for link validation errors (keeping EPIPE for format validation > > errors). > > > > My preferences for such a link validation error are (in descending order): > > > > - ENOLINK > > - EMLINK > > > > Personally I feel that if you can't validate the video pipeline, then you > > can't setup the video data links, i.e. ENOLINK. > > > > EMLINK is when you have too many links which feels wrong to me, although > > Sakari prefers it. Could this actually be a separate error? I.e. if you > > make too many links active? Perhaps we should allow both errors? > > I originally proposed EMLINK since it was POSIX while ENOLINK at some point > apparently was not, but man 3 errno now tells both are. I think we agree, > and my understanding is Laurent is fine with this as well. To summarise: > > - change v4l2_subdev_link_validate_default() return -EPIPE instead of > -EINVAL on error, > > - change media_entity_pipeline_start() to return -ENOLINK instead of -EPIPE > if link configuration error is encountered and > > - change the documentation accordingly. > > Please ack. That's fine with me. -- 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
