Re: [PATCH 2/3] media: stkwebcam: Bugfix for not correctly initialized camera

[Mauro Carvalho Chehab <mchehab+samsung@kernel.org>]

Em Wed, 5 Dec 2018 16:56:39 -0200
Mauro Carvalho Chehab <mchehab+samsung@kernel.org> escreveu:

> Em Fri, 30 Nov 2018 15:58:07 +0100
> Andreas Pape <ap@ca-pape.de> escreveu:
> 
> > Hi Kieran,
> > 
> > thanks for the review.
> > 
> > On Mon, 26 Nov 2018 12:48:08 +0000
> > Kieran Bingham <kieran.bingham@ideasonboard.com> wrote:
> > 
> > > This one worries me a little... (but hopefully not too much)
> > >  
> > 
> > As mentioned, I don't have any experience concerning video drivers;-). I found
> > this patch more or less experimentally....
> >  
> > >   
> > > > Signed-off-by: Andreas Pape <ap@ca-pape.de>
> > > > ---
> > > >  drivers/media/usb/stkwebcam/stk-webcam.c | 2 ++
> > > >  1 file changed, 2 insertions(+)
> > > > 
> > > > diff --git a/drivers/media/usb/stkwebcam/stk-webcam.c b/drivers/media/usb/stkwebcam/stk-webcam.c
> > > > index e61427e50525..c64928e36a5a 100644
> > > > --- a/drivers/media/usb/stkwebcam/stk-webcam.c
> > > > +++ b/drivers/media/usb/stkwebcam/stk-webcam.c
> > > > @@ -1155,6 +1155,8 @@ static int stk_vidioc_streamon(struct file *filp,
> > > >  	if (dev->sio_bufs == NULL)
> > > >  		return -EINVAL;
> > > >  	dev->sequence = 0;
> > > > +	stk_initialise(dev);
> > > > +	stk_setup_format(dev);  
> > > 
> > > Glancing through the code base - this seems to imply to me that s_fmt
> > > was not set/called (presumably by cheese) as stk_setup_format() is
> > > called only by stk_vidioc_s_fmt_vid_cap() and stk_camera_resume().
> > > 
> > > Is this an issue?
> > > 
> > > I presume that this means the camera will just operate in a default
> > > configuration until cheese chooses something more specific.
> > >  
> > 
> > Could be. I had a video but colours, sensitivity and possibly other things
> > were crap or at least very "psychedelic". Therefore the idea came up that
> > some kind of initialisation was missing here. 
> > 
> > > Actually - looking further this seems to be the case, as the mode is
> > > simply stored in dev->vsettings.mode, and so this last setup stage will
> > > just ensure the configuration of the hardware matches the driver.
> > > 
> > > So it seems reasonable to me - but should it be set any earlier?
> > > Perhaps not.
> > > 
> > > 
> > > Are there any complaints when running v4l2-compliance on this device node?
> > >   
> > 
> > Here is the output of v4l2-compliance:
> > 
> > v4l2-compliance SHA   : not available
> > 
> > Driver Info:
> > 	Driver name   : stk
> > 	Card type     : stk
> > 	Bus info      : usb-0000:00:1d.7-5
> > 	Driver version: 4.15.18
> > 	Capabilities  : 0x85200001
> > 		Video Capture
> > 		Read/Write
> > 		Streaming
> > 		Extended Pix Format
> > 		Device Capabilities
> > 	Device Caps   : 0x05200001
> > 		Video Capture
> > 		Read/Write
> > 		Streaming
> > 		Extended Pix Format
> > 
> > Compliance test for device /dev/video0 (not using libv4l2):
> > 
> > Required ioctls:
> > 	test VIDIOC_QUERYCAP: OK
> > 
> > Allow for multiple opens:
> > 	test second video open: OK
> > 	test VIDIOC_QUERYCAP: OK
> > 	test VIDIOC_G/S_PRIORITY: OK
> > 	test for unlimited opens: OK
> > 
> > Debug ioctls:
> > 	test VIDIOC_DBG_G/S_REGISTER: OK (Not Supported)
> > 	test VIDIOC_LOG_STATUS: OK
> > 
> > Input ioctls:
> > 	test VIDIOC_G/S_TUNER/ENUM_FREQ_BANDS: OK (Not Supported)
> > 	test VIDIOC_G/S_FREQUENCY: OK (Not Supported)
> > 	test VIDIOC_S_HW_FREQ_SEEK: OK (Not Supported)
> > 	test VIDIOC_ENUMAUDIO: OK (Not Supported)
> > 	test VIDIOC_G/S/ENUMINPUT: OK
> > 	test VIDIOC_G/S_AUDIO: OK (Not Supported)
> > 	Inputs: 1 Audio Inputs: 0 Tuners: 0
> > 
> > Output ioctls:
> > 	test VIDIOC_G/S_MODULATOR: OK (Not Supported)
> > 	test VIDIOC_G/S_FREQUENCY: OK (Not Supported)
> > 	test VIDIOC_ENUMAUDOUT: OK (Not Supported)
> > 	test VIDIOC_G/S/ENUMOUTPUT: OK (Not Supported)
> > 	test VIDIOC_G/S_AUDOUT: OK (Not Supported)
> > 	Outputs: 0 Audio Outputs: 0 Modulators: 0
> > 
> > Input/Output configuration ioctls:
> > 	test VIDIOC_ENUM/G/S/QUERY_STD: OK (Not Supported)
> > 	test VIDIOC_ENUM/G/S/QUERY_DV_TIMINGS: OK (Not Supported)
> > 	test VIDIOC_DV_TIMINGS_CAP: OK (Not Supported)
> > 	test VIDIOC_G/S_EDID: OK (Not Supported)
> > 
> > Test input 0:
> > 
> > 	Control ioctls:
> > 		test VIDIOC_QUERY_EXT_CTRL/QUERYMENU: OK
> > 		test VIDIOC_QUERYCTRL: OK
> > 		test VIDIOC_G/S_CTRL: OK
> > 		test VIDIOC_G/S/TRY_EXT_CTRLS: OK
> > 		test VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: OK
> > 		test VIDIOC_G/S_JPEGCOMP: OK (Not Supported)
> > 		Standard Controls: 4 Private Controls: 0
> > 
> > 	Format ioctls:
> > 		test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: OK
> > 		test VIDIOC_G/S_PARM: OK
> > 		test VIDIOC_G_FBUF: OK (Not Supported)
> > 		test VIDIOC_G_FMT: OK
> > 		warn: v4l2-test-formats.cpp(732): TRY_FMT cannot handle an invalid pixelformat.
> > 		warn: v4l2-test-formats.cpp(733): This may or may not be a problem. For more information see:
> > 		warn: v4l2-test-formats.cpp(734): http://www.mail-archive.com/linux-media@vger.kernel.org/msg56550.html
> > 		test VIDIOC_TRY_FMT: OK
> > 		warn: v4l2-test-formats.cpp(997): S_FMT cannot handle an invalid pixelformat.
> > 		warn: v4l2-test-formats.cpp(998): This may or may not be a problem. For more information see:
> > 		warn: v4l2-test-formats.cpp(999): http://www.mail-archive.com/linux-media@vger.kernel.org/msg56550.html
> > 		test VIDIOC_S_FMT: OK
> > 		test VIDIOC_G_SLICED_VBI_CAP: OK (Not Supported)
> > 		test Cropping: OK (Not Supported)
> > 		test Composing: OK (Not Supported)
> > 		test Scaling: OK (Not Supported)
> > 
> > 	Codec ioctls:
> > 		test VIDIOC_(TRY_)ENCODER_CMD: OK (Not Supported)
> > 		test VIDIOC_G_ENC_INDEX: OK (Not Supported)
> > 		test VIDIOC_(TRY_)DECODER_CMD: OK (Not Supported)
> > 
> > 	Buffer ioctls:
> > 		warn: v4l2-test-buffers.cpp(538): VIDIOC_CREATE_BUFS not supported
> > 		test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK
> > 		test VIDIOC_EXPBUF: OK (Not Supported)
> > 
> > Test input 0:
> > 
> > 
> > Total: 43, Succeeded: 43, Failed: 0, Warnings: 7
> 
> I'll apply this patch. On those USB cameras, the best is to always
> initialize, as we can't ensure that apps will do that.
> 
> There are actually some record scripts that just do a cat
> at the /dev/video0 device, without issuing any ioctl
> (or just doing a minimal set of them, via v4l2-ctl).

Sorry, misread the patch. It actually seems wrong to initialize it
at streamon(). That would actually break things like what I mentioned,
where a script first calls v4l2-ctl and then do something like:

	$ v4l2-ctl -d /dev/video0 -v width=640,height=480,pixelformat=YUY2
	$ cat /dev/video0 | mencoder rawvideo -rawvideo w=640:h=480:format=yuy2

(assuming that the stkwebcam driver supports it)

I'll drop this one

Thanks,
Mauro