Date:   Fri, 29 Apr 2022 12:59:20 +0000
From:   <Eugen.Hristev@...rochip.com>
To:     <hverkuil@...all.nl>, <jacopo@...ndi.org>,
        <laurent.pinchart@...asonboard.com>
CC:     <linux-media@...r.kernel.org>, <devicetree@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>,
        <linux-arm-kernel@...ts.infradead.org>,
        <Claudiu.Beznea@...rochip.com>, <robh+dt@...nel.org>,
        <Nicolas.Ferre@...rochip.com>
Subject: Re: [PATCH v9 08/13] media: atmel: atmel-isc: change format
 propagation to subdev into only verification

On 4/29/22 3:50 PM, Hans Verkuil wrote:
> On 29/04/2022 14:38, Jacopo Mondi wrote:
>> Hi
>>
>> 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@...rochip.com 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@...rochip.com>
>>>>>>>>>>>>>> Reviewed-by: Jacopo Mondi <jacopo@...ndi.org>
>>>>>>>>>>>>>> ---
>>>>>>>>>>>>>> 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)
>>
>> Otherwise, should the series go in as it is now ?
> 
> I had a comment about patch 6, so I want a new series anyway. All the
> patches with just fixes (everything but 4 and 8) can go in once I have
> them.
> 
> I think - all things considered - it is currently best to start and stop the
> pipeline in start/stop_streaming: that at least avoids all the complications
> with streamon/off/close() that is really hard to get right without (IMHO)
> my proposed vb2 changes.
> 
> With that I will accept the series.

Hi,

I will be a few days off, but next week I will address the changes and 
come up with v10. I have to test it a little to make sure everything is 
fine.

Thanks !
Eugen


> 
> Regards,
> 
>          Hans
> 
>>
>> Thanks
>>    j
>>
>>>>>> 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
> 

Powered by blists - more mailing lists