Re: [PATCH v4 4/5] media: staging: rkisp1: cap: use v4l2_pipeline_stream_{enable,disable} helpers

[Tomasz Figa <tfiga@chromium.org>]

On Wed, Jun 10, 2020 at 07:22:04PM +0200, Dafna Hirschfeld wrote:
> 
> 
> On 10.06.20 19:03, Tomasz Figa wrote:
> > Hi Dafna,
> > 
> > On Fri, May 22, 2020 at 09:55:21AM +0200, Dafna Hirschfeld wrote:
> > > From: Helen Koike <helen.koike@collabora.com>
> > > 
> > > Use v4l2_pipeline_stream_{enable,disable} to call .s_stream()
> > > subdevice callbacks through the pipeline.
> > > Those helpers are called only if the other capture is not streaming.
> > > 
> > > If the other capture is streaming then he already did that for us
> > > so we call s_stream only on the resizer that is connected to the
> > > capture node.
> > > 
> > > Signed-off-by: Helen Koike <helen.koike@collabora.com>
> > > Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> > > ---
> > >   drivers/staging/media/rkisp1/rkisp1-capture.c | 104 ++++++------------
> > >   1 file changed, 32 insertions(+), 72 deletions(-)
> > > 
> > 
> > Thank you for the patch. Please see my comments inline.
> > 
> > [snip]
> > > +static int rkisp1_s_stream_subdev(struct rkisp1_capture *cap, int enable)
> > > +{
> > > +	struct rkisp1_device *rkisp1 = cap->rkisp1;
> > > +	struct rkisp1_capture *other = &rkisp1->capture_devs[cap->id ^ 1];
> > > +	int ret;
> > > +
> > > +	/*
> > > +	 * if the other capture is already streaming then we only need to
> > > +	 * call s_stream of our reszier
> > > +	 */
> > > +	if (other->is_streaming) {
> > > +		struct v4l2_subdev *rsz_sd  = &rkisp1->resizer_devs[cap->id].sd;
> > > +
> > > +		ret = v4l2_subdev_call(rsz_sd, video, s_stream, enable);
> > > +		if (ret && ret != -ENOIOCTLCMD)
> > > +			dev_err(rkisp1->dev,
> > > +				"stream %s resizer '%s' failed (%d)\n",
> > > +				enable ? "on" : "off", rsz_sd->name, ret);
> > 
> > Do we need this special case? Wouldn't v4l2_pipeline_stream_*() simply
> > increment reference counters for the other entities?
> 
> I removed the stream count in v4 of the patchset since I thought it
> might be problematic/confusing to add a field "stream_count" in
> "struct v4l2_subdev" that is used and updated only by those helper functions
> 
> What do you think?
> 
> There is also the issue that both you and Sakari Ailus mentioned that
> an isp driver can't know the subtopology of a sensor driver and how it handle the
> s_stream callback on it's entities.

Ah, okay, so we settled on removing the refcounting from the helpers.
Fair enough. Sorry for the noise.

Feel free to add my Reviewed-by.

Best regards,
Tomasz