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 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.

Regards,
Alain

> 
> -- 
> Regards,
> 
> Laurent Pinchart
> _______________________________________________
> Linux-stm32 mailing list
> Linux-stm32@st-md-mailman.stormreply.com
> https://st-md-mailman.stormreply.com/mailman/listinfo/linux-stm32