Re: [PATCH 3/3] rcar-vin: handle transfer errors from subdevices and stop streaming if required

From: "Niklas Söderlund" <niklas.soderlund@ragnatech.se>
To: Michael Rodin <mrodin@de.adit-jv.com>
Cc: Mauro Carvalho Chehab <mchehab@kernel.org>,
	linux-media@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-renesas-soc@vger.kernel.org, michael@rodin.online,
	erosca@de.adit-jv.com
Subject: Re: [PATCH 3/3] rcar-vin: handle transfer errors from subdevices and stop streaming if required
Date: Wed, 8 Jun 2022 23:04:31 +0200	[thread overview]
Message-ID: <YqEO3/KekkZhVjW+@oden.dyn.berto.se> (raw)
In-Reply-To: <20220520195041.GA18056@vmlxhi-121.adit-jv.com>

Hi Michael,

On 2022-05-20 21:50:41 +0200, Michael Rodin wrote:

[snip]

> > 
> > Do we need to set xfer_error to false here? The delayed work is canceled 
> > and we reset the xfer_error when we start in rvin_start_streaming().
> > 
> 
> You are right, this seems to be redundant. But I think that there might be
> a different case where we have to reset xfer_error:
> 
>  1. A non-critical transfer error has occurred during streaming from a
>     HDMI source.
>  2. Frames are still captured for an hour without any further problems,
>     since it was just a short glitch
>  3. Now the source (e.g. HDMI signal generator) has been powered off by the
>     user so it does not send new frames.
>  4. Timeout occurs due to 3 but since xfer_error has been set 1 hour ago,
>     userspace is notified about a transfer error and assumes that streaming
>     has been stopped because of this.
> 
> To avoid this scenario I think maybe we have to restrict validity of
> xfer_error. Maybe it would be better to make xfer_error a counter which is
> set after a transfer error to e.g. 10 frames and then decremented after
> each captured frame so after 10 successfully captured frames we know that a
> timeout has occurred definitely not due to a transfer error?
> 
> Another possible improvement might be to make FRAME_TIMEOUT_MS configurable,
> maybe via a v4l2 control from userspace? Or we could also define the timeout
> as a multiple of the frame interval of the source. This would allow us to
> reduce the timeout further based on the particular source so the userspace
> does not have to wait for a second until it knows that it has to restart
> streaming.
> 
> What do you think?

I discussed this problem last week at a conference and the consensus was 
that this problem of timeouts and the like should in the first hand be 
handled in user-space. The reason being that there might be use-cases 
that are better dealt with there.

If the monitor thread is is strictly needed for some reason in kernel 
thread it should likely be moved to the V4L2 core as all drivers would 
then be able to use it instead of deeding on slightly different 
implementations in each driver.

So I fear we are back to only try to signal xfer errors in the driver 
and then leave it to either user-space or some new V4L2 code to help 
monitoring.

Sorry for only understanding this so late in the review, it took some 
time for me to understand it but once explained to me it made sens.

-- 
Kind Regards,
Niklas Söderlund