Re: [Linux-stm32] [PATCH v1 3/5] media: stm32-dcmipp: STM32 DCMIPP camera interface driver

[Alain Volmat <alain.volmat@foss.st.com>]

Hi Laurent,

On Tue, Aug 29, 2023 at 11:26:35AM +0300, Laurent Pinchart wrote:
> On Thu, Aug 24, 2023 at 06:05:06PM +0200, Alain Volmat wrote:
> > Hi Laurent,
> > 
> > On Thu, Aug 24, 2023 at 04:04:32PM +0300, Laurent Pinchart wrote:
> > > On Thu, Aug 24, 2023 at 12:26:42PM +0000, Sakari Ailus wrote:
> > > > On Thu, Aug 24, 2023 at 01:09:34PM +0200, Alain Volmat wrote:
> > > > > Hi Sakari,
> > > > > 
> > > > > thanks a lot for the review.  I've already taken care of the comments I got
> > > > > from Dan and will also add fixes for your comments as well before
> > > > > pushing the v2.  Before going into that I thought I'd better clarify the
> > > > > framerate part which seems the most tricky part.
> > > > > 
> > > > > On Mon, Aug 07, 2023 at 09:29:55AM +0000, Sakari Ailus wrote:
> > > > 
> > > > ...
> > > > 
> > > > > > > +static int dcmipp_byteproc_g_frame_interval(struct v4l2_subdev *sd,
> > > > > > > +					    struct v4l2_subdev_frame_interval *fi)
> > > > > > > +{
> > > > > > > +	struct dcmipp_byteproc_device *byteproc = v4l2_get_subdevdata(sd);
> > > > > > > +
> > > > > > > +	if (IS_SINK(fi->pad))
> > > > > > > +		fi->interval = byteproc->sink_interval;
> > > > > > > +	else
> > > > > > > +		fi->interval = byteproc->src_interval;
> > > > > > > +
> > > > > > > +	return 0;
> > > > > > > +}
> > > > > > > +
> > > > > > > +static int dcmipp_byteproc_s_frame_interval(struct v4l2_subdev *sd,
> > > > > > > +					    struct v4l2_subdev_frame_interval *fi)
> > > > > > > +{
> > > > > > > +	struct dcmipp_byteproc_device *byteproc = v4l2_get_subdevdata(sd);
> > > > > > > +
> > > > > > > +	mutex_lock(&byteproc->lock);
> > > > > > > +
> > > > > > > +	if (byteproc->streaming) {
> > > > > > > +		mutex_unlock(&byteproc->lock);
> > > > > > > +		return -EBUSY;
> > > > > > > +	}
> > > > > > > +
> > > > > > > +	if (fi->interval.numerator == 0 || fi->interval.denominator == 0)
> > > > > > > +		fi->interval = byteproc->sink_interval;
> > > > > > > +
> > > > > > > +	if (IS_SINK(fi->pad)) {
> > > > > > > +		/*
> > > > > > > +		 * Setting sink frame interval resets frame skipping.
> > > > > > > +		 * Sink frame interval is propagated to src.
> > > > > > > +		 */
> > > > > > > +		byteproc->frate = 0;
> > > > > > > +		byteproc->sink_interval = fi->interval;
> > > > > > > +		byteproc->src_interval = byteproc->sink_interval;
> > > > > > 
> > > > > > Is this used for anything else than configure skipping?
> > > > > > 
> > > > > > I think I'd just have a control for it in that case.
> > > > > > 
> > > > > > I don't think exposing frame interval configuration is necessarily even
> > > > > > meaningful for a device that just processes data but does not produce it.
> > > > > 
> > > > > The DCMIPP is able to perform frame drop, 1/2, 1/4, 1/8 basically.
> > > > > As Dan pointed me out, indeed setting frame interval as we did on both
> > > > > sink and source pad isn't a defined behavior.  I first thought that
> > > > > using the frame interval was the proper way to do that but that is
> > > > > indeed only used on producers such as sensors ....
> > > > > Which ctrl would you propose in such case ?
> > > > 
> > > > We don't have one, AFAIK, and I think it may be unlikely this will be
> > > > needed elsewhere. So I'd use a private control.
> > > > 
> > > > I wonder what others think. Cc Laurent as well.
> > > 
> > > What are the use cases for this feature ?
> > 
> > This is basically to allow reducing the framerate of the
> > captured stream when this is not possible at the producer
> > (sensor) level and we need to lower down the stress on elements down the
> > pipeline.
> 
> I wonder if the frame interval API is a good fit for this. The driver
> accepts frame rates of 1, 2, 4 and 8 fps and maps them to the
> corresponding decimation factor, these values are not the actual frame
> rate.

Not exactly.  The way it was done was that we allow setting frame
interval on both sink and source pad of the subdev and compute the frame
skip based on the those two framerates.
As an example, if the framerate on sink pad is 1/30 and the one on the
source pad is 1/15 we will skip half of the frames.  The frame interval
setted via the pads were real framerates.
The issue here is that it seems there is no such driver implementing frame
rate adjustments in the kernel and moreover the V4L2 subdev spec says that
frame interval control should only be done on a single pad of a subdev.

Dan proposed as an alternative to only keep the frame interval setting
on the source pad of the subdev and have the subdev driver retrieve the
stream framerate via inter subdev calls down to a subdev implementing the
get frame interval.

Regards,
Alain