Re: [PATCH v9 08/13] media: atmel: atmel-isc: change format propagation to subdev into only verification

[Laurent Pinchart]

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.

-- 
Regards,

Laurent Pinchart