* [PATCH 0/3] Fix for webcam issues with ASUS A6VM
@ 2018-11-23 16:14 Andreas Pape
2018-11-23 16:14 ` [PATCH 1/3] media: stkwebcam: Support for ASUS A6VM notebook added Andreas Pape
` (2 more replies)
0 siblings, 3 replies; 11+ messages in thread
From: Andreas Pape @ 2018-11-23 16:14 UTC (permalink / raw)
To: linux-media, kieran.bingham; +Cc: Andreas Pape
This set of patches makes the Syntek USB webcam integrated into Asus A6VM
notebooks accessible again.
Andreas Pape (3):
media: stkwebcam: Support for ASUS A6VM notebook added.
media: stkwebcam: Bugfix for not correctly initialized camera
media: stkwebcam: Bugfix for wrong return values
drivers/media/usb/stkwebcam/stk-webcam.c | 15 ++++++++++++++-
1 file changed, 14 insertions(+), 1 deletion(-)
--
2.17.1
^ permalink raw reply [flat|nested] 11+ messages in thread* [PATCH 1/3] media: stkwebcam: Support for ASUS A6VM notebook added. 2018-11-23 16:14 [PATCH 0/3] Fix for webcam issues with ASUS A6VM Andreas Pape @ 2018-11-23 16:14 ` Andreas Pape 2018-11-26 12:48 ` Kieran Bingham 2018-11-23 16:14 ` [PATCH 2/3] media: stkwebcam: Bugfix for not correctly initialized camera Andreas Pape 2018-11-23 16:14 ` [PATCH 3/3] media: stkwebcam: Bugfix for wrong return values Andreas Pape 2 siblings, 1 reply; 11+ messages in thread From: Andreas Pape @ 2018-11-23 16:14 UTC (permalink / raw) To: linux-media, kieran.bingham; +Cc: Andreas Pape The ASUS A6VM notebook has a built in stk11xx webcam which is mounted in a way that the video is vertically and horizontally flipped. Therefore this notebook is added to the special handling in the driver to automatically flip the video into the correct orientation. Signed-off-by: Andreas Pape <ap@ca-pape.de> --- drivers/media/usb/stkwebcam/stk-webcam.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/drivers/media/usb/stkwebcam/stk-webcam.c b/drivers/media/usb/stkwebcam/stk-webcam.c index e11d5d5b7c26..e61427e50525 100644 --- a/drivers/media/usb/stkwebcam/stk-webcam.c +++ b/drivers/media/usb/stkwebcam/stk-webcam.c @@ -116,6 +116,13 @@ static const struct dmi_system_id stk_upside_down_dmi_table[] = { DMI_MATCH(DMI_PRODUCT_NAME, "T12Rg-H") } }, + { + .ident = "ASUS A6VM", + .matches = { + DMI_MATCH(DMI_SYS_VENDOR, "ASUSTeK Computer Inc."), + DMI_MATCH(DMI_PRODUCT_NAME, "A6VM") + } + }, {} }; -- 2.17.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 1/3] media: stkwebcam: Support for ASUS A6VM notebook added. 2018-11-23 16:14 ` [PATCH 1/3] media: stkwebcam: Support for ASUS A6VM notebook added Andreas Pape @ 2018-11-26 12:48 ` Kieran Bingham 2018-11-30 15:07 ` Andreas Pape 0 siblings, 1 reply; 11+ messages in thread From: Kieran Bingham @ 2018-11-26 12:48 UTC (permalink / raw) To: Andreas Pape, linux-media Hi Andreas, Thank you for the patch, On 23/11/2018 16:14, Andreas Pape wrote: > The ASUS A6VM notebook has a built in stk11xx webcam which is mounted > in a way that the video is vertically and horizontally flipped. > Therefore this notebook is added to the special handling in the driver > to automatically flip the video into the correct orientation. > > Signed-off-by: Andreas Pape <ap@ca-pape.de> > --- > drivers/media/usb/stkwebcam/stk-webcam.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/drivers/media/usb/stkwebcam/stk-webcam.c b/drivers/media/usb/stkwebcam/stk-webcam.c > index e11d5d5b7c26..e61427e50525 100644 > --- a/drivers/media/usb/stkwebcam/stk-webcam.c > +++ b/drivers/media/usb/stkwebcam/stk-webcam.c > @@ -116,6 +116,13 @@ static const struct dmi_system_id stk_upside_down_dmi_table[] = { > DMI_MATCH(DMI_PRODUCT_NAME, "T12Rg-H") > } > }, > + { > + .ident = "ASUS A6VM", > + .matches = { > + DMI_MATCH(DMI_SYS_VENDOR, "ASUSTeK Computer Inc."), > + DMI_MATCH(DMI_PRODUCT_NAME, "A6VM") > + } I guess these strings match the strings produced by dmi-decode on your laptop? Assuming so: Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > + }, > {} > }; > > -- Regards -- Kieran ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/3] media: stkwebcam: Support for ASUS A6VM notebook added. 2018-11-26 12:48 ` Kieran Bingham @ 2018-11-30 15:07 ` Andreas Pape 0 siblings, 0 replies; 11+ messages in thread From: Andreas Pape @ 2018-11-30 15:07 UTC (permalink / raw) To: kieran.bingham; +Cc: linux-media Hi Keiran, thanks for the review. On Mon, 26 Nov 2018 12:48:53 +0000 Kieran Bingham <kieran.bingham@ideasonboard.com> wrote: > > I guess these strings match the strings produced by dmi-decode on your > laptop? > I didn't use dmidecode but I read the values from /sys/class/dmi/sys_vendor and /sys/class/dmi/product_name accordingly. Kind regards, Andreas ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 2/3] media: stkwebcam: Bugfix for not correctly initialized camera 2018-11-23 16:14 [PATCH 0/3] Fix for webcam issues with ASUS A6VM Andreas Pape 2018-11-23 16:14 ` [PATCH 1/3] media: stkwebcam: Support for ASUS A6VM notebook added Andreas Pape @ 2018-11-23 16:14 ` Andreas Pape 2018-11-26 12:48 ` Kieran Bingham 2018-11-23 16:14 ` [PATCH 3/3] media: stkwebcam: Bugfix for wrong return values Andreas Pape 2 siblings, 1 reply; 11+ messages in thread From: Andreas Pape @ 2018-11-23 16:14 UTC (permalink / raw) To: linux-media, kieran.bingham; +Cc: Andreas Pape stk_start_stream can only be called successfully if stk_initialise and stk_setup_format are called before. When using e.g. cheese it was observed that stk_initialise and stk_setup_format have not been called before which leads to no picture in that software whereas other tools like guvcview worked flawlessly. This patch solves the issue when using e.g. cheese. 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); return stk_start_stream(dev); } -- 2.17.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 2/3] media: stkwebcam: Bugfix for not correctly initialized camera 2018-11-23 16:14 ` [PATCH 2/3] media: stkwebcam: Bugfix for not correctly initialized camera Andreas Pape @ 2018-11-26 12:48 ` Kieran Bingham 2018-11-30 14:58 ` Andreas Pape 0 siblings, 1 reply; 11+ messages in thread From: Kieran Bingham @ 2018-11-26 12:48 UTC (permalink / raw) To: Andreas Pape, linux-media Hi Andreas, Thank you for the patch, On 23/11/2018 16:14, Andreas Pape wrote: > stk_start_stream can only be called successfully if stk_initialise and > stk_setup_format are called before. When using e.g. cheese it was observed > that stk_initialise and stk_setup_format have not been called before which > leads to no picture in that software whereas other tools like guvcview > worked flawlessly. This patch solves the issue when using e.g. cheese. > This one worries me a little... (but hopefully not too much) > 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. 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? > return stk_start_stream(dev); > } > > -- Regards -- Kieran ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/3] media: stkwebcam: Bugfix for not correctly initialized camera 2018-11-26 12:48 ` Kieran Bingham @ 2018-11-30 14:58 ` Andreas Pape 2018-12-05 18:56 ` Mauro Carvalho Chehab 0 siblings, 1 reply; 11+ messages in thread From: Andreas Pape @ 2018-11-30 14:58 UTC (permalink / raw) To: kieran.bingham; +Cc: linux-media 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 Kind regards, Andreas -- Andreas Pape <ap@ca-pape.de> ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/3] media: stkwebcam: Bugfix for not correctly initialized camera 2018-11-30 14:58 ` Andreas Pape @ 2018-12-05 18:56 ` Mauro Carvalho Chehab 2018-12-05 19:08 ` Mauro Carvalho Chehab 0 siblings, 1 reply; 11+ messages in thread From: Mauro Carvalho Chehab @ 2018-12-05 18:56 UTC (permalink / raw) To: Andreas Pape; +Cc: kieran.bingham, linux-media 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). Thanks, Mauro ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/3] media: stkwebcam: Bugfix for not correctly initialized camera 2018-12-05 18:56 ` Mauro Carvalho Chehab @ 2018-12-05 19:08 ` Mauro Carvalho Chehab 0 siblings, 0 replies; 11+ messages in thread From: Mauro Carvalho Chehab @ 2018-12-05 19:08 UTC (permalink / raw) To: Andreas Pape; +Cc: kieran.bingham, linux-media 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 3/3] media: stkwebcam: Bugfix for wrong return values 2018-11-23 16:14 [PATCH 0/3] Fix for webcam issues with ASUS A6VM Andreas Pape 2018-11-23 16:14 ` [PATCH 1/3] media: stkwebcam: Support for ASUS A6VM notebook added Andreas Pape 2018-11-23 16:14 ` [PATCH 2/3] media: stkwebcam: Bugfix for not correctly initialized camera Andreas Pape @ 2018-11-23 16:14 ` Andreas Pape 2018-11-26 12:33 ` Kieran Bingham 2 siblings, 1 reply; 11+ messages in thread From: Andreas Pape @ 2018-11-23 16:14 UTC (permalink / raw) To: linux-media, kieran.bingham; +Cc: Andreas Pape usb_control_msg returns in case of a successfully sent message the number of sent bytes as a positive number. Don't use this value as a return value for stk_camera_read_reg, as a non-zero return value is used as an error condition in some cases when stk_camera_read_reg is called. Signed-off-by: Andreas Pape <ap@ca-pape.de> --- drivers/media/usb/stkwebcam/stk-webcam.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/drivers/media/usb/stkwebcam/stk-webcam.c b/drivers/media/usb/stkwebcam/stk-webcam.c index c64928e36a5a..66a3665fc826 100644 --- a/drivers/media/usb/stkwebcam/stk-webcam.c +++ b/drivers/media/usb/stkwebcam/stk-webcam.c @@ -171,7 +171,11 @@ int stk_camera_read_reg(struct stk_camera *dev, u16 index, u8 *value) *value = *buf; kfree(buf); - return ret; + + if (ret < 0) + return ret; + else + return 0; } static int stk_start_stream(struct stk_camera *dev) -- 2.17.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 3/3] media: stkwebcam: Bugfix for wrong return values 2018-11-23 16:14 ` [PATCH 3/3] media: stkwebcam: Bugfix for wrong return values Andreas Pape @ 2018-11-26 12:33 ` Kieran Bingham 0 siblings, 0 replies; 11+ messages in thread From: Kieran Bingham @ 2018-11-26 12:33 UTC (permalink / raw) To: Andreas Pape, linux-media Hi Andreas, Thank you for the patch, On 23/11/2018 16:14, Andreas Pape wrote: > usb_control_msg returns in case of a successfully sent message the number > of sent bytes as a positive number. Don't use this value as a return value > for stk_camera_read_reg, as a non-zero return value is used as an error > condition in some cases when stk_camera_read_reg is called. Yes, and I see the stk_camera_write_reg() also follows this pattern. > Signed-off-by: Andreas Pape <ap@ca-pape.de> > --- > drivers/media/usb/stkwebcam/stk-webcam.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/drivers/media/usb/stkwebcam/stk-webcam.c b/drivers/media/usb/stkwebcam/stk-webcam.c > index c64928e36a5a..66a3665fc826 100644 > --- a/drivers/media/usb/stkwebcam/stk-webcam.c > +++ b/drivers/media/usb/stkwebcam/stk-webcam.c > @@ -171,7 +171,11 @@ int stk_camera_read_reg(struct stk_camera *dev, u16 index, u8 *value) > *value = *buf; > > kfree(buf); > - return ret; > + > + if (ret < 0) > + return ret; > + else > + return 0; I would have said the return 0; could be on it's own, and the else statement would then not be needed, but I see this follows the style used by stk_camera_write_reg() - so it looks good to me. Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > } > > static int stk_start_stream(struct stk_camera *dev) > -- Regards -- Kieran ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2018-12-05 19:08 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-11-23 16:14 [PATCH 0/3] Fix for webcam issues with ASUS A6VM Andreas Pape 2018-11-23 16:14 ` [PATCH 1/3] media: stkwebcam: Support for ASUS A6VM notebook added Andreas Pape 2018-11-26 12:48 ` Kieran Bingham 2018-11-30 15:07 ` Andreas Pape 2018-11-23 16:14 ` [PATCH 2/3] media: stkwebcam: Bugfix for not correctly initialized camera Andreas Pape 2018-11-26 12:48 ` Kieran Bingham 2018-11-30 14:58 ` Andreas Pape 2018-12-05 18:56 ` Mauro Carvalho Chehab 2018-12-05 19:08 ` Mauro Carvalho Chehab 2018-11-23 16:14 ` [PATCH 3/3] media: stkwebcam: Bugfix for wrong return values Andreas Pape 2018-11-26 12:33 ` Kieran Bingham
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).