On Fri, Apr 29, 2022 at 02:38:59PM +0200, Jacopo Mondi wrote: > On Fri, Apr 29, 2022 at 03:28:11PM +0300, Laurent Pinchart wrote: > > On Fri, Apr 29, 2022 at 02:23:45PM +0200, Hans Verkuil wrote: > > > On 29/04/2022 14:02, Laurent Pinchart wrote: > > > > On Fri, Apr 29, 2022 at 01:17:27PM +0200, Hans Verkuil wrote: > > > >> On 29/04/2022 12:20, Laurent Pinchart wrote: > > > >>> On Fri, Apr 29, 2022 at 12:13:46PM +0200, Hans Verkuil wrote: > > > >>>> On 29/04/2022 12:07, Laurent Pinchart wrote: > > > >>>>> On Fri, Apr 29, 2022 at 11:58:48AM +0200, Jacopo Mondi wrote: > > > >>>>>> On Fri, Apr 29, 2022 at 10:43:09AM +0200, Hans Verkuil wrote: > > > >>>>>>> On 29/04/2022 10:28, Eugen.Hristev@xxxxxxxxxxxxx wrote: > > > >>>>>>>> On 4/29/22 11:17 AM, Hans Verkuil wrote: > > > >>>>>>>>> On 10/03/2022 10:51, Eugen Hristev wrote: > > > >>>>>>>>>> As a top MC video driver, the atmel-isc should not propagate the format to the > > > >>>>>>>>>> subdevice, it should rather check at start_streaming() time if the subdev is properly > > > >>>>>>>>>> configured with a compatible format. > > > >>>>>>>>>> Removed the whole format finding logic, and reworked the format verification > > > >>>>>>>>>> at start_streaming time, such that the ISC will return an error if the subdevice > > > >>>>>>>>>> is not properly configured. To achieve this, media_pipeline_start > > > >>>>>>>>>> is called and a link_validate callback is created to check the formats. > > > >>>>>>>>>> With this being done, the module parameter 'sensor_preferred' makes no sense > > > >>>>>>>>>> anymore. The ISC should not decide which format the sensor is using. The > > > >>>>>>>>>> ISC should only cope with the situation and inform userspace if the streaming > > > >>>>>>>>>> is possible in the current configuration. > > > >>>>>>>>>> The redesign of the format propagation has also risen the question of the > > > >>>>>>>>>> enumfmt callback. If enumfmt is called with an mbus_code, the enumfmt handler > > > >>>>>>>>>> should only return the formats that are supported for this mbus_code. > > > >>>>>>>>>> Otherwise, the enumfmt will report all the formats that the ISC could output. > > > >>>>>>>>>> With this rework, the dynamic list of user formats is removed. It makes no > > > >>>>>>>>>> more sense to identify at complete time which formats the sensor could emit, > > > >>>>>>>>>> and add those into a separate dynamic list. > > > >>>>>>>>>> The ISC will start with a simple preconfigured default format, and at > > > >>>>>>>>>> link validate time, decide whether it can use the format that is configured > > > >>>>>>>>>> on the sink or not. > > > >>>>>>>>>> > > > >>>>>>>>>> Signed-off-by: Eugen Hristev <eugen.hristev@xxxxxxxxxxxxx> > > > >>>>>>>>>> Reviewed-by: Jacopo Mondi <jacopo@xxxxxxxxxx> > > > >>>>>>>>>> --- > > > >>>>>>>>>> Changes in v9: > > > >>>>>>>>>> - isc_link_validate now static > > > >>>>>>>>>> > > > >>>>>>>>>> Changes in v7: > > > >>>>>>>>>> - minor typos as suggested by Jacopo > > > >>>>>>>>>> - small changes, reduce some indentation, modified an index, as suggested by > > > >>>>>>>>>> Jacopo > > > >>>>>>>>>> > > > >>>>>>>>>> Changes in v6: > > > >>>>>>>>>> - reworked a bit enum_fmt as suggested by Jacopo > > > >>>>>>>>>> > > > >>>>>>>>>> Changes in v5: > > > >>>>>>>>>> - removed user_formats dynamic list as it is now pointless > > > >>>>>>>>>> - greatly simplified the enum_fmt function > > > >>>>>>>>>> - removed some init code that was useless now > > > >>>>>>>>>> > > > >>>>>>>>>> Changes in v4: > > > >>>>>>>>>> - moved validation code into link_validate and used media_pipeline_start > > > >>>>>>>>>> - merged this patch with the enum_fmt patch which was previously in v3 of > > > >>>>>>>>>> the series > > > >>>>>>>>>> > > > >>>>>>>>>> Changes in v3: > > > >>>>>>>>>> - clamp to maximum resolution once the frame size from the subdev is found > > > >>>>>>>>>> drivers/media/platform/atmel/atmel-isc-base.c | 412 ++++++++---------- > > > >>>>>>>>>> .../media/platform/atmel/atmel-isc-scaler.c | 5 + > > > >>>>>>>>>> drivers/media/platform/atmel/atmel-isc.h | 13 +- > > > >>>>>>>>>> .../media/platform/atmel/atmel-sama5d2-isc.c | 20 + > > > >>>>>>>>>> .../media/platform/atmel/atmel-sama7g5-isc.c | 20 + > > > >>>>>>>>>> 5 files changed, 236 insertions(+), 234 deletions(-) > > > >>>>>>>>>> > > > >>>>>>>>>> diff --git a/drivers/media/platform/atmel/atmel-isc-base.c b/drivers/media/platform/atmel/atmel-isc-base.c > > > >>>>>>>>>> index ee1dda6707a0..fe2c0af58060 100644 > > > >>>>>>>>>> --- a/drivers/media/platform/atmel/atmel-isc-base.c > > > >>>>>>>>>> +++ b/drivers/media/platform/atmel/atmel-isc-base.c > > > >>>>>>>>>> @@ -36,11 +36,6 @@ static unsigned int debug; > > > >>>>>>>>>> module_param(debug, int, 0644); > > > >>>>>>>>>> MODULE_PARM_DESC(debug, "debug level (0-2)"); > > > >>>>>>>>>> > > > >>>>>>>>>> -static unsigned int sensor_preferred = 1; > > > >>>>>>>>>> -module_param(sensor_preferred, uint, 0644); > > > >>>>>>>>>> -MODULE_PARM_DESC(sensor_preferred, > > > >>>>>>>>>> - "Sensor is preferred to output the specified format (1-on 0-off), default 1"); > > > >>>>>>>>>> - > > > >>>>>>>>>> #define ISC_IS_FORMAT_RAW(mbus_code) \ > > > >>>>>>>>>> (((mbus_code) & 0xf000) == 0x3000) > > > >>>>>>>>>> > > > >>>>>>>>>> @@ -337,6 +332,10 @@ static int isc_start_streaming(struct vb2_queue *vq, unsigned int count) > > > >>>>>>>>>> unsigned long flags; > > > >>>>>>>>>> int ret; > > > >>>>>>>>>> > > > >>>>>>>>>> + ret = media_pipeline_start(&isc->video_dev.entity, &isc->mpipe); > > > >>>>>>>>> > > > >>>>>>>>> The pipeline validation is done in start_streaming, but I don't think that > > > >>>>>>>>> is the best place: if STREAMON is called before buffers are queued, then > > > >>>>>>>>> an invalid pipeline isn't discovered until enough buffers are queued to > > > >>>>>>>>> kick off start_streaming. > > > >>>>>>>>> > > > >>>>>>>>> Drivers like vsp1, omap3isp and the samsung drivers all do this in streamon(). > > > >>>>>>>>> > > > >>>>>>>>> I think that is the correct time to do this. > > > >>>>>>>> > > > >>>>>>>> Hello Hans, > > > >>>>>>>> > > > >>>>>>>> Initially (v2, v3) I had this in streamon(). The problem that I faced at > > > >>>>>>>> that time was that streamoff was never called, so I could not call > > > >>>>>>>> media_pipeline_stop(). Then Jacopo told me to move it to start_streaming > > > >>>>>>>> (see change log for v4) , and I did not face any more problems. > > > >>>>>> > > > >>>>>> Yes indeed, seems I suggested to use media_pipeline_handler in a > > > >>>>>> comment on your v3 > > > >>>>>> > > > >>>>>> "at s_stream time your top driver calls media_pipeline_start()" > > > >>>>>> > > > >>>>>> sorry about that, I should have looked around a bit more carefully and > > > >>>>>> notice most drivers do so at vb2 streamon > > > >>>>>> > > > >>>>>> However I don't see media_pipeline_start being called at all in v3 of > > > >>>>>> the patch > > > >>>>>> > > > >>>>>>> It's a mess. Looking at some drivers I see that omap3isp calls media_pipeline_stop > > > >>>>>>> in streamoff (so will have the same problem as you described if VIDIOC_STREAMOFF > > > >>>>>>> isn't called), exynos4-is does the same, but it also checks the streaming state in > > > >>>>>>> the release() fop callback, so that would fix this problem. And vimc does this > > > >>>>>>> in stop_streaming. > > > >>>>>>> > > > >>>>>>> I'm in favor of fixing this in vb2, that framework knows exactly when this needs > > > >>>>>>> to be called. > > > >>>>>> > > > >>>>>> Are you suggesting to have vb2 to call media_pipeline_start() or is it > > > >>>>>> more complex than this ? > > > >>>>> > > > >>>>> I think Hans meant adding a .validate() operation to vb2. > > > >>>>> > > > >>>>> vb2 is already quite complex, I don't think adding more features is a > > > >>>>> good idea. I'd rather have vb2 focus on buffer management only > > > >>>>> (.start_streaming() and .stop_streaming() shouldn't have been in there > > > >>>>> in my opinion), and handle validation in the .streamon() handler. I'd > > > >>>>> expect most drivers that deal with media pipelines to do more work in > > > >>>>> .streamon() anyway. > > > >>>> > > > >>>> I disagree with that :-) > > > >>>> > > > >>>> It's vb2 that keeps track of the streaming state and when what actions > > > >>>> need to be taken. Drivers really shouldn't need to care about the ioctls > > > >>>> themselves, and just implement the relevant vb2 callbacks. Relying on > > > >>>> drivers to handle any of the streaming ioctls is asking for problems, > > > >>>> as this shows: most drivers implement this wrong today. > > > >>>> > > > >>>> The vb2 framework knows when e.g. the pipeline needs to be started or > > > >>>> stopped, and can do this at the best time, without drivers needing to > > > >>>> keep track of when streamon/off/release is called. Keep that logic in > > > >>>> vb2. > > > >>> > > > >>> Pipeline management and buffer management are two different issues. > > > >>> Don't forget about devices that have multiple video nodes, part of the > > > >>> same pipeline (possibly a combination of output and capture nodes, or > > > >>> all of the same type). Forcing drivers to go through vb2 operations to > > > >>> handle the pipeline will be messy, will result in more bloat in vb2, and > > > >>> make the result more bug-prone and harder to maintain. > > > >>> > > > >>> If pipeline management is too complex, let's simplify it, new helpers > > > >>> can make sense, but not through vb2. > > > >> > > > >> But it is vb2 that knows when streaming starts and stops. > > > > > > > > That's right, but pipeline start (which includes validation and resource > > > > reservation) needs to be performed synchronously with VIDIOC_STREAMON. > > > > The streaming state managed by vb2 is not relevant, > > > > media_pipeline_start() must not be delayed the same way > > > > .start_streaming() is. > > > > > > It will be the first thing that vb2_streamon calls. This has nothing to do > > > with start_streaming: that's called when sufficient number of buffers are > > > queued up to be able to start the DMA. This proposed prepare_streaming op > > > will be called when VIDIOC_STREAMON is called. > > > > Then it doesn't need vb2's knowledge of the stream state :-) > > > > > >> The driver just > > > >> needs to be informed (e.g. prepare_streaming and unprepare_streaming ops). > > > >> > > > >> vb2 deals with buffer management and it keeps track of the streaming state > > > >> and makes the streaming state transitions. That *is* an integral part of > > > >> vb2. What is missing at the moment are callbacks done at streamon time and > > > >> when the streaming stops (streamoff, or close() when is_streaming is true). > > > >> > > > >> If you want to implement stream validation in a driver, then there are a > > > >> lot of things you need to do: > > > >> > > > >> - override streamon, make sure you call vb2_queue_is_busy(), validate the > > > >> pipeline, then call vb2_streamon, if that fails, remember to stop the > > > >> pipeline. > > > >> > > > >> - override streamoff, make sure you call vb2_queue_is_busy(), stop the > > > >> pipeline and call vb2_streamoff. > > > >> > > > >> - in the release() function when the fh is closed, you have to check > > > >> vb2_is_streaming(), check that you are the owner of the queue, and if true, > > > >> stop the pipeline. > > > > > > > > I'm not opposed to helper functions to implement that, they can bundle > > > > vb2 calls and pipeline management. > > > > > > > >> By moving this to vb2 ops all you need to implement are the prepare and > > > >> unprepare ops. > > > >> > > > >> Esp. the release() implementation is tricky. I'm pretty sure that > > > >> drivers/media/platform/samsung/exynos4-is/fimc-lite.c is wrong, since it > > > >> should only call media_pipeline_stop() for the owner of the queue. Instead > > > >> it calls it for the last user of the queue. > > > >> > > > >> I see that fimc_lite_streamoff() is wrong too: you can safely call > > > >> VIDIOC_STREAMOFF twice: the second streamoff just returns 0 without > > > >> doing anything. Instead media_pipeline_stop is called without testing if > > > >> the queue is streaming. > > > >> > > > >> And yes, this is in part because V4L2 has quite some history and certainly > > > >> API choice were made in the past that we wouldn't make today. But vb2 > > > >> shields you from that, and behaves much more like a proper state machine. > > > >> > > > >> I know you prefer to give a lot more control to driver developers, but > > > >> in my experience very few developers can do things like this right. And > > > >> it is really hard as a reviewer to check if all the corner cases are handled > > > >> correctly in a driver. If vb2 is used, then I know things are called at the > > > >> right time, and that makes my life as reviewer so much easier. > > > > > > > > It's not just about giving more control to drivers, it's about > > > > organizing the software layers in a way that keeps them maintainable, > > > > with layered abstractions and not midlayers. > > > > > > > > We are extensively reworking the media pipeline management as part of > > > > the stream series, and there will be more work on top of that that will > > > > make even more fundamental changes. I would like to at least postpone > > > > any work on vb2 until then, to be able to evaluate the impact. > > > > > > I'll make an RFC patch for vb2 so you have a better idea of what it does. > > > > As long as we don't merge it before I get the chance to send the media > > pipeline management rework, I'm all for RFCs :-) > > > > To unblock Eugen is it fine if he moves media_pipeline_start() at > streamon() time for now ? It will require overriding > v4l2_ioctl.vidioc_streamon which might be a bit of additional work > (but probably easier to replace once a proper solution lands) Fine with me. > Otherwise, should the series go in as it is now ? > > > > >> There may still be a few drivers that really need to do this manually, and > > > >> that's OK, but a driver like the atmel-isc doesn't need that at all. -- Regards, Laurent Pinchart
