Re: [PATCH 3/4] rcar-vin: Stop stream when subdevice signal transfer error

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Hans,

On 2021-11-08 18:36:25 +0100, Hans Verkuil wrote:
> On 08/11/2021 17:02, Niklas Söderlund wrote:
> > When a subdevice signals a transfer error stop the VIN in addition to
> > informing user-space of the event.
> > 
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@xxxxxxxxxxxx>
> > Reviewed-by: Jacopo Mondi <jacopo+renesas@xxxxxxxxxx>
> > ---
> > * Changes since v3
> > - Switch to new V4L2_EVENT_XFER_ERROR from V4L2_EVENT_EOS.
> > - Call vb2_queue_error() when encountering the event.
> > 
> > * Changes since v2
> > - Log using vin_dbg() instead of v4l2_info().
> > ---
> >  drivers/media/platform/rcar-vin/rcar-v4l2.c | 17 ++++++++++++++++-
> >  1 file changed, 16 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> > index a5bfa76fdac6e55a..bf17fdefe90aabf5 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> > +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> > @@ -992,9 +992,24 @@ void rvin_v4l2_unregister(struct rvin_dev *vin)
> >  static void rvin_notify_video_device(struct rvin_dev *vin,
> >  				     unsigned int notification, void *arg)
> >  {
> > +	const struct v4l2_event *event;
> > +
> >  	switch (notification) {
> >  	case V4L2_DEVICE_NOTIFY_EVENT:
> > -		v4l2_event_queue(&vin->vdev, arg);
> > +		event = arg;
> > +
> > +		switch (event->type) {
> > +		case V4L2_EVENT_XFER_ERROR:
> > +			vin_dbg(vin,
> > +				"Subdevice signaled transfer error, stopping.\n");
> > +			rvin_stop_streaming(vin);
> > +			vb2_queue_error(&vin->queue);
> 
> Hmm, wouldn't it be the case that every driver that calls vb2_queue_error()
> would also have to send this new event? Would it be possible to modify
> vb2_queue_error() to raise this event? I haven't analyzed all the drivers
> that call this function to see if that makes sense.
> 
> Perhaps a separate new function vb2_queue_error_with_event() would also be
> an option.

I think that maybe a good idea, but I think that would be needed on-top 
of this work as I can't really test it. Here the rcar-csi2.ko is a 
subdevice which detects the error condition and generates the event. And 
this code is in rcar-vin.ko, the video device driver which reacts to the 
event and then forwards it to user-space.

Or am I misunderstanding you? And you think I should remove the 
v4l2_event_queue() below in favor of a new vb2_queue_error_with_event() 
call?

> 
> Regards,
> 
> 	Hans
> 
> > +			break;
> > +		default:
> > +			break;
> > +		}
> > +
> > +		v4l2_event_queue(&vin->vdev, event);
> >  		break;
> >  	default:
> >  		break;
> > 
> 

-- 
Kind Regards,
Niklas Söderlund



[Index of Archives]     [Linux Samsung SOC]     [Linux Wireless]     [Linux Kernel]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Device Mapper]

  Powered by Linux