cron job: media_tree daily build: ABI WARNING
This message is generated daily by a cron job that builds media_tree for the kernels and architectures in the list below. Results of the daily build of media_tree: date: Tue Sep 13 04:00:20 CEST 2016 git branch: test git hash: c3b809834db8b1a8891c7ff873a216eac119628d gcc version:i686-linux-gcc (GCC) 5.4.0 sparse version: v0.5.0-56-g7647c77 smatch version: v0.5.0-3428-gdfe27cf host hardware: x86_64 host os:4.6.0-164 linux-git-arm-at91: OK linux-git-arm-davinci: OK linux-git-arm-multi: OK linux-git-arm-pxa: OK linux-git-blackfin-bf561: OK linux-git-i686: OK linux-git-m32r: OK linux-git-mips: OK linux-git-powerpc64: OK linux-git-sh: OK linux-git-x86_64: OK linux-2.6.36.4-i686: OK linux-2.6.37.6-i686: OK linux-2.6.38.8-i686: OK linux-2.6.39.4-i686: OK linux-3.0.60-i686: OK linux-3.1.10-i686: OK linux-3.2.37-i686: OK linux-3.3.8-i686: OK linux-3.4.27-i686: OK linux-3.5.7-i686: OK linux-3.6.11-i686: OK linux-3.7.4-i686: OK linux-3.8-i686: OK linux-3.9.2-i686: OK linux-3.10.1-i686: OK linux-3.11.1-i686: OK linux-3.12.23-i686: OK linux-3.13.11-i686: OK linux-3.14.9-i686: OK linux-3.15.2-i686: OK linux-3.16.7-i686: OK linux-3.17.8-i686: OK linux-3.18.7-i686: OK linux-3.19-i686: OK linux-4.0-i686: OK linux-4.1.1-i686: OK linux-4.2-i686: OK linux-4.3-i686: OK linux-4.4-i686: OK linux-4.5-i686: OK linux-4.6-i686: OK linux-4.7-i686: OK linux-4.8-rc1-i686: OK linux-2.6.36.4-x86_64: OK linux-2.6.37.6-x86_64: OK linux-2.6.38.8-x86_64: OK linux-2.6.39.4-x86_64: OK linux-3.0.60-x86_64: OK linux-3.1.10-x86_64: OK linux-3.2.37-x86_64: OK linux-3.3.8-x86_64: OK linux-3.4.27-x86_64: OK linux-3.5.7-x86_64: OK linux-3.6.11-x86_64: OK linux-3.7.4-x86_64: OK linux-3.8-x86_64: OK linux-3.9.2-x86_64: OK linux-3.10.1-x86_64: OK linux-3.11.1-x86_64: OK linux-3.12.23-x86_64: OK linux-3.13.11-x86_64: OK linux-3.14.9-x86_64: OK linux-3.15.2-x86_64: OK linux-3.16.7-x86_64: OK linux-3.17.8-x86_64: OK linux-3.18.7-x86_64: OK linux-3.19-x86_64: OK linux-4.0-x86_64: OK linux-4.1.1-x86_64: OK linux-4.2-x86_64: OK linux-4.3-x86_64: OK linux-4.4-x86_64: OK linux-4.5-x86_64: OK linux-4.6-x86_64: OK linux-4.7-x86_64: OK linux-4.8-rc1-x86_64: OK apps: WARNINGS spec-git: OK ABI WARNING: change for arm-pxa sparse: WARNINGS smatch: WARNINGS Detailed results are available here: http://www.xs4all.nl/~hverkuil/logs/Tuesday.log Full logs are available here: http://www.xs4all.nl/~hverkuil/logs/Tuesday.tar.bz2 The Media Infrastructure API from this daily build is here: http://www.xs4all.nl/~hverkuil/spec/index.html -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] [media] atmel-isc: mark PM functions as __maybe_unused
Hi Arnd,
Thank you for your patch.
I think it's better to add switch CONFIG_PM, but the PM feature is a
must, or the ISC can not work, maybe the best choice is to add 'depends
on PM' in Kconfig.
#ifdef CONFIG_PM
isc_runtime_suspend
{
XXX
}
isc_runtime_resume
{
XXX
}
static const struct dev_pm_ops atmel_isc_dev_pm_ops = {
SET_RUNTIME_PM_OPS(isc_runtime_suspend, isc_runtime_resume, NULL)
};
#define ATMEL_ISC_PM_OPS(&atmel_isc_dev_pm_ops)
#else
#define ATMEL_ISC_PM_OPSNULL
#endif
static struct platform_driver atmel_isc_driver = {
.probe = atmel_isc_probe,
.remove = atmel_isc_remove,
.driver = {
.name = ATMEL_ISC_NAME,
.pm = ATMEL_ISC_PM_OPS,
.of_match_table = of_match_ptr(atmel_isc_of_match),
},
};
On 9/12/2016 23:32, Arnd Bergmann wrote:
The newly added atmel-isc driver uses SET_RUNTIME_PM_OPS() to
refer to its suspend/resume functions, causing a warning when
CONFIG_PM is not set:
media/platform/atmel/atmel-isc.c:1477:12: error: 'isc_runtime_resume' defined
but not used [-Werror=unused-function]
media/platform/atmel/atmel-isc.c:1467:12: error: 'isc_runtime_suspend' defined
but not used [-Werror=unused-function]
This adds __maybe_unused annotations to avoid the warning without
adding an error-prone #ifdef around it.
Signed-off-by: Arnd Bergmann
---
drivers/media/platform/atmel/atmel-isc.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/media/platform/atmel/atmel-isc.c
b/drivers/media/platform/atmel/atmel-isc.c
index db6773de92f0..a9ab7ae89f04 100644
--- a/drivers/media/platform/atmel/atmel-isc.c
+++ b/drivers/media/platform/atmel/atmel-isc.c
@@ -1464,7 +1464,7 @@ static int atmel_isc_remove(struct platform_device *pdev)
return 0;
}
-static int isc_runtime_suspend(struct device *dev)
+static int __maybe_unused isc_runtime_suspend(struct device *dev)
{
struct isc_device *isc = dev_get_drvdata(dev);
@@ -1474,7 +1474,7 @@ static int isc_runtime_suspend(struct device *dev)
return 0;
}
-static int isc_runtime_resume(struct device *dev)
+static int __maybe_unused isc_runtime_resume(struct device *dev)
{
struct isc_device *isc = dev_get_drvdata(dev);
int ret;
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/2] [media] tw5864 constify some structures
On Tue, Sep 13, 2016 at 02:02:36AM +0300, Andrey Utkin wrote: > tw5864 is a recently submitted driver and it is currently present only > in media tree. Oops, have just fetched linux-next updates, tw5864 is already in next. -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/2] [media] tw5864: constify vb2_ops structure
Inspired by "[media] pci: constify vb2_ops structures" patch
from Julia Lawall (dated 9 Sep 2016).
Signed-off-by: Andrey Utkin
---
drivers/media/pci/tw5864/tw5864-video.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/media/pci/tw5864/tw5864-video.c
b/drivers/media/pci/tw5864/tw5864-video.c
index 6c1685a..7401b64 100644
--- a/drivers/media/pci/tw5864/tw5864-video.c
+++ b/drivers/media/pci/tw5864/tw5864-video.c
@@ -465,7 +465,7 @@ static void tw5864_stop_streaming(struct vb2_queue *q)
spin_unlock_irqrestore(&input->slock, flags);
}
-static struct vb2_ops tw5864_video_qops = {
+static const struct vb2_ops tw5864_video_qops = {
.queue_setup = tw5864_queue_setup,
.buf_queue = tw5864_buf_queue,
.start_streaming = tw5864_start_streaming,
--
2.9.2
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/2] [media] tw5864: constify struct video_device template
tw5864_video_template is used for filling of actual video_device
structures. It is copied by value, and is not used for anything else.
Signed-off-by: Andrey Utkin
---
drivers/media/pci/tw5864/tw5864-video.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/media/pci/tw5864/tw5864-video.c
b/drivers/media/pci/tw5864/tw5864-video.c
index 7401b64..652a059 100644
--- a/drivers/media/pci/tw5864/tw5864-video.c
+++ b/drivers/media/pci/tw5864/tw5864-video.c
@@ -912,7 +912,7 @@ static const struct v4l2_ioctl_ops video_ioctl_ops = {
#endif
};
-static struct video_device tw5864_video_template = {
+static const struct video_device tw5864_video_template = {
.name = "tw5864_video",
.fops = &video_fops,
.ioctl_ops = &video_ioctl_ops,
--
2.9.2
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 0/2] [media] tw5864 constify some structures
tw5864 is a recently submitted driver and it is currently present only in media tree. Recent patches submitted by Julia Lawall urged me to make similar changes in this driver. Andrey Utkin (2): [media] tw5864: constify vb2_ops structure [media] tw5864: constify struct video_device template drivers/media/pci/tw5864/tw5864-video.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) -- 2.9.2 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 00/26] constify local structures
On Mon, 12 Sep 2016, Jarkko Sakkinen wrote: > On Mon, Sep 12, 2016 at 04:43:58PM +0300, Felipe Balbi wrote: > > > > Hi, > > > > Jarkko Sakkinen writes: > > > On Mon, Sep 12, 2016 at 10:54:07AM +0200, Julia Lawall wrote: > > >> > > >> > > >> On Sun, 11 Sep 2016, Jarkko Sakkinen wrote: > > >> > > >> > On Sun, Sep 11, 2016 at 03:05:42PM +0200, Julia Lawall wrote: > > >> > > Constify local structures. > > >> > > > > >> > > The semantic patch that makes this change is as follows: > > >> > > (http://coccinelle.lip6.fr/) > > >> > > > >> > Just my two cents but: > > >> > > > >> > 1. You *can* use a static analysis too to find bugs or other issues. > > >> > 2. However, you should manually do the commits and proper commit > > >> >messages to subsystems based on your findings. And I generally think > > >> >that if one contributes code one should also at least smoke test > > >> > changes > > >> >somehow. > > >> > > > >> > I don't know if I'm alone with my opinion. I just think that one should > > >> > also do the analysis part and not blindly create and submit patches. > > >> > > >> All of the patches are compile tested. And the individual patches are > > > > > > Compile-testing is not testing. If you are not able to test a commit, > > > you should explain why. > > > > Dude, Julia has been doing semantic patching for years already and > > nobody has raised any concerns so far. There's already an expectation > > that Coccinelle *works* and Julia's sematic patches are sound. > > > > Besides, adding 'const' is something that causes virtually no functional > > changes to the point that build-testing is really all you need. Any > > problems caused by adding 'const' to a definition will be seen by build > > errors or warnings. > > > > Really, just stop with the pointless discussion and go read a bit about > > Coccinelle and what semantic patches are giving you. The work done by > > Julia and her peers are INRIA have measurable benefits. > > > > You're really making a thunderstorm in a glass of water. > > Hmm... I've been using coccinelle in cyclic basis for some time now. > My comment was oversized but I didn't mean it to be impolite or attack > of any kind for that matter. No problem :) Thanks for the feedback. julia -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 00/26] constify local structures
On Mon, Sep 12, 2016 at 04:43:58PM +0300, Felipe Balbi wrote: > > Hi, > > Jarkko Sakkinen writes: > > On Mon, Sep 12, 2016 at 10:54:07AM +0200, Julia Lawall wrote: > >> > >> > >> On Sun, 11 Sep 2016, Jarkko Sakkinen wrote: > >> > >> > On Sun, Sep 11, 2016 at 03:05:42PM +0200, Julia Lawall wrote: > >> > > Constify local structures. > >> > > > >> > > The semantic patch that makes this change is as follows: > >> > > (http://coccinelle.lip6.fr/) > >> > > >> > Just my two cents but: > >> > > >> > 1. You *can* use a static analysis too to find bugs or other issues. > >> > 2. However, you should manually do the commits and proper commit > >> >messages to subsystems based on your findings. And I generally think > >> >that if one contributes code one should also at least smoke test > >> > changes > >> >somehow. > >> > > >> > I don't know if I'm alone with my opinion. I just think that one should > >> > also do the analysis part and not blindly create and submit patches. > >> > >> All of the patches are compile tested. And the individual patches are > > > > Compile-testing is not testing. If you are not able to test a commit, > > you should explain why. > > Dude, Julia has been doing semantic patching for years already and > nobody has raised any concerns so far. There's already an expectation > that Coccinelle *works* and Julia's sematic patches are sound. > > Besides, adding 'const' is something that causes virtually no functional > changes to the point that build-testing is really all you need. Any > problems caused by adding 'const' to a definition will be seen by build > errors or warnings. > > Really, just stop with the pointless discussion and go read a bit about > Coccinelle and what semantic patches are giving you. The work done by > Julia and her peers are INRIA have measurable benefits. > > You're really making a thunderstorm in a glass of water. Hmm... I've been using coccinelle in cyclic basis for some time now. My comment was oversized but I didn't mean it to be impolite or attack of any kind for that matter. > -- > balbi /Jarkko -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: musb: isoc pkt loss with pwc
2016-09-12 21:57 GMT+03:00 Bin Liu :
> Hi,
>
> On Mon, Sep 12, 2016 at 11:52:46AM +0300, Matwey V. Kornilov wrote:
>> 2016-09-12 6:28 GMT+03:00 Bin Liu :
>> > Hi,
>> >
>> > On Tue, Aug 30, 2016 at 11:44:33PM +0300, Matwey V. Kornilov wrote:
>> >> 2016-08-30 21:30 GMT+03:00 Bin Liu :
>> >> > Hi,
>> >> >
>> >> > On Sun, Aug 28, 2016 at 01:13:55PM +0300, Matwey V. Kornilov wrote:
>> >> >> Hello Bin,
>> >> >>
>> >> >> I would like to start new thread on my issue. Let me recall where the
>> >> >> issue is:
>> >> >> There is 100% frame lost in pwc webcam driver due to lots of
>> >> >> zero-length packages coming from musb driver.
>> >> >
>> >> > What is the video resolution and fps?
>> >>
>> >> 640x480 YUV420 10 frames per second.
>> >> pwc uses proprietary compression during device-host transmission, but
>> >> I don't know how effective it is.
>> >
>> > The data rate for VGA YUV420 @10fps is 640x480*1.5*10 = 4.6MB/s, which
>> > is much higher than full-speed 12Mbps. So the video data on the bus is
>> > compressed, not YUV420, I believe.
>> >
>> >>
>> >> >
>> >> >> The issue is present in all kernels (including 4.8) starting from the
>> >> >> commit:
>> >> >>
>> >> >> f551e13529833e052f75ec628a8af7b034af20f9 ("Revert "usb: musb:
>> >> >> musb_host: Enable HCD_BH flag to handle urb return in bottom half"")
>> >> >
>> >> > What is the behavior without this commit?
>> >>
>> >> Without this commit all frames are being received correctly. Single
>> >
>> > Which means without this commit your camera has been working without
>> > issues, and this is a regression with this commit, right?
>> >
>>
>> Right
>
> Okay, thanks for confirming.
>
> But we cannot just simply add this flag, as it breaks many other use
> cases. I will continue work on this to find a solution which works on
> all use cases.
>
Ok, thank you.
> Regards,
> -Bin.
>
--
With best regards,
Matwey V. Kornilov.
Sternberg Astronomical Institute, Lomonosov Moscow State University, Russia
119991, Moscow, Universitetsky pr-k 13, +7 (495) 9392382
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: musb: isoc pkt loss with pwc
Hi,
On Mon, Sep 12, 2016 at 11:52:46AM +0300, Matwey V. Kornilov wrote:
> 2016-09-12 6:28 GMT+03:00 Bin Liu :
> > Hi,
> >
> > On Tue, Aug 30, 2016 at 11:44:33PM +0300, Matwey V. Kornilov wrote:
> >> 2016-08-30 21:30 GMT+03:00 Bin Liu :
> >> > Hi,
> >> >
> >> > On Sun, Aug 28, 2016 at 01:13:55PM +0300, Matwey V. Kornilov wrote:
> >> >> Hello Bin,
> >> >>
> >> >> I would like to start new thread on my issue. Let me recall where the
> >> >> issue is:
> >> >> There is 100% frame lost in pwc webcam driver due to lots of
> >> >> zero-length packages coming from musb driver.
> >> >
> >> > What is the video resolution and fps?
> >>
> >> 640x480 YUV420 10 frames per second.
> >> pwc uses proprietary compression during device-host transmission, but
> >> I don't know how effective it is.
> >
> > The data rate for VGA YUV420 @10fps is 640x480*1.5*10 = 4.6MB/s, which
> > is much higher than full-speed 12Mbps. So the video data on the bus is
> > compressed, not YUV420, I believe.
> >
> >>
> >> >
> >> >> The issue is present in all kernels (including 4.8) starting from the
> >> >> commit:
> >> >>
> >> >> f551e13529833e052f75ec628a8af7b034af20f9 ("Revert "usb: musb:
> >> >> musb_host: Enable HCD_BH flag to handle urb return in bottom half"")
> >> >
> >> > What is the behavior without this commit?
> >>
> >> Without this commit all frames are being received correctly. Single
> >
> > Which means without this commit your camera has been working without
> > issues, and this is a regression with this commit, right?
> >
>
> Right
Okay, thanks for confirming.
But we cannot just simply add this flag, as it breaks many other use
cases. I will continue work on this to find a solution which works on
all use cases.
Regards,
-Bin.
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 00/26] constify local structures
On Mon, Sep 12, 2016 at 03:52:08PM +0200, Julia Lawall wrote: > > > On Mon, 12 Sep 2016, Felipe Balbi wrote: > > > > > Hi, > > > > Jarkko Sakkinen writes: > > > On Mon, Sep 12, 2016 at 10:54:07AM +0200, Julia Lawall wrote: > > >> > > >> > > >> On Sun, 11 Sep 2016, Jarkko Sakkinen wrote: > > >> > > >> > On Sun, Sep 11, 2016 at 03:05:42PM +0200, Julia Lawall wrote: > > >> > > Constify local structures. > > >> > > > > >> > > The semantic patch that makes this change is as follows: > > >> > > (http://coccinelle.lip6.fr/) > > >> > > > >> > Just my two cents but: > > >> > > > >> > 1. You *can* use a static analysis too to find bugs or other issues. > > >> > 2. However, you should manually do the commits and proper commit > > >> >messages to subsystems based on your findings. And I generally think > > >> >that if one contributes code one should also at least smoke test > > >> > changes > > >> >somehow. > > >> > > > >> > I don't know if I'm alone with my opinion. I just think that one should > > >> > also do the analysis part and not blindly create and submit patches. > > >> > > >> All of the patches are compile tested. And the individual patches are > > > > > > Compile-testing is not testing. If you are not able to test a commit, > > > you should explain why. > > > > Dude, Julia has been doing semantic patching for years already and > > nobody has raised any concerns so far. There's already an expectation > > that Coccinelle *works* and Julia's sematic patches are sound. > > > > Besides, adding 'const' is something that causes virtually no functional > > changes to the point that build-testing is really all you need. Any > > problems caused by adding 'const' to a definition will be seen by build > > errors or warnings. > > > > Really, just stop with the pointless discussion and go read a bit about > > Coccinelle and what semantic patches are giving you. The work done by > > Julia and her peers are INRIA have measurable benefits. > > > > You're really making a thunderstorm in a glass of water. > > Thanks for the defense, but since a lot of these patches torned out to be > wrong, due to an incorrect parse by Coccinelle, combined with an > unpleasantly lax compiler, Jarkko does have a point that I should have > looked at the patches more carefully. In any case, I have written to the > maintainers relevant to the patches that turned out to be incorrect. Exactly. I'm not excepting that every commit would require extensive analysis but it would be good to quickly at least skim through commits and see if they make sense (or ask if unsure) :) And I'm fine with compile testing if it is mentioned in the commit msg. > julia /Jarkko -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] [media] ad5820: use __maybe_unused for PM functions
On Mon 2016-09-12 17:32:57, Arnd Bergmann wrote:
> The new ad5820 driver uses #ifdef to hide the suspend/resume functions,
> but gets it wrong when CONFIG_PM_SLEEP is disabled:
>
> drivers/media/i2c/ad5820.c:286:12: error: 'ad5820_resume' defined but not
> used [-Werror=unused-function]
> drivers/media/i2c/ad5820.c:274:12: error: 'ad5820_suspend' defined but not
> used [-Werror=unused-function]
>
> This replaces the #ifdef with a __maybe_unused annotation that is
> simpler and harder to get wrong, avoiding the warning.
>
> Signed-off-by: Arnd Bergmann
> Fixes: bee3d5115611 ("[media] ad5820: Add driver for auto-focus
coil")
Thanks!
Acked-by: Pavel Machek
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures)
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v1 2/2] st-hva: add debug file system
This patch creates 4 static debugfs entries to dump:
- the device-related information ("st-hva/device")
- the list of registered encoders ("st-hva/encoders")
- the current values of the hva registers ("st-hva/regs")
- the information about the last closed instance ("st-hva/last")
It also creates dynamically a debugfs entry for each opened instance,
("st-hva/") to dump:
- the information about the stream (profile, level, resolution,
alignment...)
- the control parameters (bitrate mode, framerate, GOP size...)
- the potential (system, encoding...) errors
- the performance information about the encoding (HW processing
duration, average bitrate, average framerate...)
Each time a running instance is closed, its context (including the
debug information) is saved to feed, on demand, the last closed
instance debugfs entry.
Signed-off-by: Yannick Fertre
Signed-off-by: Jean-Christophe Trotin
---
drivers/media/platform/sti/hva/hva-debug.c | 362 +
drivers/media/platform/sti/hva/hva-hw.c| 39
drivers/media/platform/sti/hva/hva-hw.h| 1 +
drivers/media/platform/sti/hva/hva-v4l2.c | 11 +-
drivers/media/platform/sti/hva/hva.h | 86 +--
5 files changed, 482 insertions(+), 17 deletions(-)
diff --git a/drivers/media/platform/sti/hva/hva-debug.c
b/drivers/media/platform/sti/hva/hva-debug.c
index 71bbf32..433b1d4 100644
--- a/drivers/media/platform/sti/hva/hva-debug.c
+++ b/drivers/media/platform/sti/hva/hva-debug.c
@@ -5,7 +5,113 @@
* License terms: GNU General Public License (GPL), version 2
*/
+#include
+
#include "hva.h"
+#include "hva-hw.h"
+
+static void format_ctx(struct seq_file *s, struct hva_ctx *ctx)
+{
+ struct hva_streaminfo *stream = &ctx->streaminfo;
+ struct hva_frameinfo *frame = &ctx->frameinfo;
+ struct hva_controls *ctrls = &ctx->ctrls;
+ struct hva_ctx_dbg *dbg = &ctx->dbg;
+ u32 bitrate_mode, aspect, entropy, vui_sar, sei_fp;
+
+ seq_printf(s, "|-%s\n |\n", ctx->name);
+
+ seq_printf(s, " |-[%sframe info]\n",
+ ctx->flags & HVA_FLAG_FRAMEINFO ? "" : "default ");
+ seq_printf(s, " | |- pixel format=%4.4s\n"
+ " | |- wxh=%dx%d\n"
+ " | |- wxh (w/ encoder alignment constraint)=%dx%d\n"
+ " |\n",
+ (char *)&frame->pixelformat,
+ frame->width, frame->height,
+ frame->aligned_width, frame->aligned_height);
+
+ seq_printf(s, " |-[%sstream info]\n",
+ ctx->flags & HVA_FLAG_STREAMINFO ? "" : "default ");
+ seq_printf(s, " | |- stream format=%4.4s\n"
+ " | |- wxh=%dx%d\n"
+ " | |- %s\n"
+ " | |- %s\n"
+ " |\n",
+ (char *)&stream->streamformat,
+ stream->width, stream->height,
+ stream->profile, stream->level);
+
+ bitrate_mode = V4L2_CID_MPEG_VIDEO_BITRATE_MODE;
+ aspect = V4L2_CID_MPEG_VIDEO_ASPECT;
+ seq_puts(s, " |-[parameters]\n");
+ seq_printf(s, " | |- %s\n"
+ " | |- bitrate=%d bps\n"
+ " | |- GOP size=%d\n"
+ " | |- video aspect=%s\n"
+ " | |- framerate=%d/%d\n",
+ v4l2_ctrl_get_menu(bitrate_mode)[ctrls->bitrate_mode],
+ ctrls->bitrate,
+ ctrls->gop_size,
+ v4l2_ctrl_get_menu(aspect)[ctrls->aspect],
+ ctrls->time_per_frame.denominator,
+ ctrls->time_per_frame.numerator);
+
+ entropy = V4L2_CID_MPEG_VIDEO_H264_ENTROPY_MODE;
+ vui_sar = V4L2_CID_MPEG_VIDEO_H264_VUI_SAR_IDC;
+ sei_fp = V4L2_CID_MPEG_VIDEO_H264_SEI_FP_ARRANGEMENT_TYPE;
+ if (stream->streamformat == V4L2_PIX_FMT_H264) {
+ seq_printf(s, " | |- %s entropy mode\n"
+ " | |- CPB size=%d kB\n"
+ " | |- DCT8x8 enable=%s\n"
+ " | |- qpmin=%d\n"
+ " | |- qpmax=%d\n"
+ " | |- PAR enable=%s\n"
+ " | |- PAR id=%s\n"
+ " | |- SEI frame packing enable=%s\n"
+ " | |- SEI frame packing type=%s\n",
+ v4l2_ctrl_get_menu(entropy)[ctrls->entropy_mode],
+ ctrls->cpb_size,
+ ctrls->dct8x8 ? "true" : "false",
+ ctrls->qpmin,
+ ctrls->qpmax,
+ ctrls->vui_sar ? "true" : "false",
+ v4l2_ctrl_get_menu(vui_sar)[ctrls->vui_sar_idc],
+ ctrls->sei_fp ? "true" : "false",
+ v4l2_ctrl_get_menu(sei_fp)[ct
[PATCH v1 0/2] add debug capabilities to v4l2 encoder for STMicroelectronics SOC
version 1:
- Initial submission
With the first patch, a short summary about the encoding operation is
unconditionnaly printed at each instance closing:
- information about the stream (format, profile, level, resolution)
- performance information (number of encoded frames, maximum framerate)
- potential (system, encoding...) errors
With the second patch, 4 static debugfs entries are created to dump:
- the device-related information ("st-hva/device")
- the list of registered encoders ("st-hva/encoders")
- the current values of the hva registers ("st-hva/regs")
- the information about the last closed instance ("st-hva/last")
Moreover, a debugfs entry is dynamically created for each opened instance,
("st-hva/") to dump:
- the information about the stream (profile, level, resolution,
alignment...)
- the control parameters (bitrate mode, framerate, GOP size...)
- the potential (system, encoding...) errors
- the performance information about the encoding (HW processing
duration, average bitrate, average framerate...)
Each time a running instance is closed, its context (including the debug
information) is saved to feed, on demand, the last closed instance debugfs
entry.
These debug capabilities are mainly implemented in the hva-debug.c file.
Jean-Christophe Trotin (2):
st-hva: encoding summary at instance release
st-hva: add debug file system
drivers/media/platform/sti/hva/Makefile| 2 +-
drivers/media/platform/sti/hva/hva-debug.c | 487 +
drivers/media/platform/sti/hva/hva-h264.c | 6 +
drivers/media/platform/sti/hva/hva-hw.c| 44 +++
drivers/media/platform/sti/hva/hva-hw.h| 1 +
drivers/media/platform/sti/hva/hva-mem.c | 5 +-
drivers/media/platform/sti/hva/hva-v4l2.c | 41 ++-
drivers/media/platform/sti/hva/hva.h | 85 -
8 files changed, 656 insertions(+), 15 deletions(-)
create mode 100644 drivers/media/platform/sti/hva/hva-debug.c
--
1.9.1
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v1 1/2] st-hva: encoding summary at instance release
This patch prints unconditionnaly a short summary about the encoding
operation at each instance closing, for debug purpose:
- information about the stream (format, profile, level, resolution)
- performance information (number of encoded frames, maximum framerate)
- potential (system, encoding...) errors
Signed-off-by: Yannick Fertre
Signed-off-by: Jean-Christophe Trotin
---
drivers/media/platform/sti/hva/Makefile| 2 +-
drivers/media/platform/sti/hva/hva-debug.c | 125 +
drivers/media/platform/sti/hva/hva-h264.c | 6 ++
drivers/media/platform/sti/hva/hva-hw.c| 5 ++
drivers/media/platform/sti/hva/hva-mem.c | 5 +-
drivers/media/platform/sti/hva/hva-v4l2.c | 30 ---
drivers/media/platform/sti/hva/hva.h | 27 +++
7 files changed, 188 insertions(+), 12 deletions(-)
create mode 100644 drivers/media/platform/sti/hva/hva-debug.c
diff --git a/drivers/media/platform/sti/hva/Makefile
b/drivers/media/platform/sti/hva/Makefile
index ffb69ce..0895d2d 100644
--- a/drivers/media/platform/sti/hva/Makefile
+++ b/drivers/media/platform/sti/hva/Makefile
@@ -1,2 +1,2 @@
obj-$(CONFIG_VIDEO_STI_HVA) := st-hva.o
-st-hva-y := hva-v4l2.o hva-hw.o hva-mem.o hva-h264.o
+st-hva-y := hva-v4l2.o hva-hw.o hva-mem.o hva-h264.o hva-debug.o
diff --git a/drivers/media/platform/sti/hva/hva-debug.c
b/drivers/media/platform/sti/hva/hva-debug.c
new file mode 100644
index 000..71bbf32
--- /dev/null
+++ b/drivers/media/platform/sti/hva/hva-debug.c
@@ -0,0 +1,125 @@
+/*
+ * Copyright (C) STMicroelectronics SA 2015
+ * Authors: Yannick Fertre
+ * Hugues Fruchet
+ * License terms: GNU General Public License (GPL), version 2
+ */
+
+#include "hva.h"
+
+/*
+ * encoding summary
+ */
+
+char *hva_dbg_summary(struct hva_ctx *ctx)
+{
+ struct hva_streaminfo *stream = &ctx->streaminfo;
+ struct hva_frameinfo *frame = &ctx->frameinfo;
+ struct hva_ctx_dbg *dbg = &ctx->dbg;
+ static char str[200] = "";
+ char *cur = str;
+ size_t left = sizeof(str);
+ int cnt = 0;
+ int ret = 0;
+ u32 errors;
+
+ /* frame info */
+ cur += cnt;
+ left -= cnt;
+ ret = snprintf(cur, left, "%4.4s %dx%d > ",
+ (char *)&frame->pixelformat,
+ frame->aligned_width, frame->aligned_height);
+ cnt = (left > ret ? ret : left);
+
+ /* stream info */
+ cur += cnt;
+ left -= cnt;
+ ret = snprintf(cur, left, "%4.4s %dx%d %s %s: ",
+ (char *)&stream->streamformat,
+ stream->width, stream->height,
+ stream->profile, stream->level);
+ cnt = (left > ret ? ret : left);
+
+ /* performance info */
+ cur += cnt;
+ left -= cnt;
+ ret = snprintf(cur, left, "%d frames encoded", dbg->cnt_duration);
+ cnt = (left > ret ? ret : left);
+
+ if (dbg->cnt_duration && dbg->total_duration) {
+ u64 div;
+ u32 fps;
+
+ div = (u64)dbg->cnt_duration * 10;
+ do_div(div, dbg->total_duration);
+ fps = (u32)div;
+ cur += cnt;
+ left -= cnt;
+ ret = snprintf(cur, left, ", max fps (0.1Hz)=%d", fps);
+ cnt = (left > ret ? ret : left);
+ }
+
+ /* error info */
+ errors = dbg->sys_errors + dbg->encode_errors + dbg->frame_errors;
+ if (errors) {
+ cur += cnt;
+ left -= cnt;
+ ret = snprintf(cur, left, ", %d errors", errors);
+ cnt = (left > ret ? ret : left);
+ }
+
+ return str;
+}
+
+/*
+ * performance debug info
+ */
+
+void hva_dbg_perf_begin(struct hva_ctx *ctx)
+{
+ struct hva_ctx_dbg *dbg = &ctx->dbg;
+
+ dbg->begin = ktime_get();
+
+ /*
+* filter sequences valid for performance:
+* - begin/begin (no stream available) is an invalid sequence
+* - begin/end is a valid sequence
+*/
+ dbg->is_valid_period = false;
+}
+
+void hva_dbg_perf_end(struct hva_ctx *ctx, struct hva_stream *stream)
+{
+ struct device *dev = ctx_to_dev(ctx);
+ u64 div;
+ u32 duration;
+ u32 bytesused;
+ u32 timestamp;
+ struct hva_ctx_dbg *dbg = &ctx->dbg;
+ ktime_t end = ktime_get();
+
+ /* stream bytesused and timestamp in us */
+ bytesused = vb2_get_plane_payload(&stream->vbuf.vb2_buf, 0);
+ div = stream->vbuf.vb2_buf.timestamp;
+ do_div(div, 1000);
+ timestamp = (u32)div;
+
+ /* encoding duration */
+ div = (u64)ktime_us_delta(end, dbg->begin);
+
+ dev_dbg(dev,
+ "%s perf stream[%d] dts=%d encoded using %d bytes in %d us",
+ ctx->name,
+ stream->vbuf.sequence,
+ timestamp,
+ bytesused, (u32)div);
+
+ do_div(div, 100);
+ duration = (u32)div;
+
+ dbg->total_dura
Re: [PATCH v2] [media] vivid: support for contiguous DMA buffers
Hello Vincent, On Mon, Sep 12, 2016 at 4:47 AM, Vincent Abriou wrote: > It allows to simulate the behavior of hardware with such limitations or > to connect vivid to real hardware with such limitations. > > Add the "allocators" module parameter option to let vivid use the > dma-contig instead of vmalloc. > > Signed-off-by: Philipp Zabel > Signed-off-by: Hans Verkuil > Signed-off-by: Vincent Abriou > > Cc: Philipp Zabel > Cc: Hans Verkuil > --- The patch looks good to me. Reviewed-by: Javier Martinez Canillas I've also tested on an Exynos5 board to share DMA buffers between a vivid capture device and the Exynos DRM driver, so: Tested-by: Javier Martinez Canillas Before $SUBJECT, when vivid was always using the vb2 vmalloc memory allocator, the Exynos DRM driver wasn't able to import the dma-buf because the GEM buffers are non-contiguous: $ gst-launch-1.0 v4l2src device=/dev/video7 io-mode=dmabuf ! kmssink Setting pipeline to PAUSED ... Pipeline is live and does not need PREROLL ... Setting pipeline to PLAYING ... New clock: GstSystemClock 0:00:00.853895814 29570xd6260 ERROR kmsallocator gstkmsallocator.c:334:gst_kms_allocator_add_fb: Failed to bind to framebuffer: Invalid argument (-22) [ 1757.390564] [drm:exynos_drm_framebuffer_init] *ERROR* cannot use this gem memory type for fb. The issue goes away when using the the vb2 DMA contig memory allocator. Best regards, Javier -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/2] [media] Input: synaptics-rmi4: disallow impossible configuration
The newly added debug mode for the synaptics-rmi4 driver relies on
the v4l2 interface and vb2_vmalloc, but those might be configured
as loadable modules when the driver itself is built-in, resulting
in a link failure:
drivers/input/rmi4/rmi_core.o: In function `rmi_f54_remove':
rmi_f54.c:(.text.rmi_f54_remove+0x14): undefined reference to
`video_unregister_device'
rmi_f54.c:(.text.rmi_f54_remove+0x20): undefined reference to
`v4l2_device_unregister'
drivers/input/rmi4/rmi_core.o: In function `rmi_f54_vidioc_s_input':
rmi_f54.c:(.text.rmi_f54_vidioc_s_input+0x10): undefined reference to
`video_devdata'
drivers/input/rmi4/rmi_core.o: In function `rmi_f54_vidioc_g_input':
rmi_f54.c:(.text.rmi_f54_vidioc_g_input+0x10): undefined reference to
`video_devdata'
drivers/input/rmi4/rmi_core.o: In function `rmi_f54_vidioc_fmt':
rmi_f54.c:(.text.rmi_f54_vidioc_fmt+0x10): undefined reference to
`video_devdata'
drivers/input/rmi4/rmi_core.o: In function `rmi_f54_vidioc_enum_input':
rmi_f54.c:(.text.rmi_f54_vidioc_enum_input+0x10): undefined reference to
`video_devdata'
drivers/input/rmi4/rmi_core.o: In function `rmi_f54_vidioc_querycap':
...
The best workaround I could come up with is to disallow the debug
mode unless it's actually possible to call it.
Signed-off-by: Arnd Bergmann
Fixes: 3a762dbd5347 ("[media] Input: synaptics-rmi4 - add support for F54
diagnostics")
---
drivers/input/rmi4/Kconfig | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/input/rmi4/Kconfig b/drivers/input/rmi4/Kconfig
index f3418b65eb41..4c8a55857e00 100644
--- a/drivers/input/rmi4/Kconfig
+++ b/drivers/input/rmi4/Kconfig
@@ -65,7 +65,7 @@ config RMI4_F30
config RMI4_F54
bool "RMI4 Function 54 (Analog diagnostics)"
depends on RMI4_CORE
- depends on VIDEO_V4L2
+ depends on VIDEO_V4L2=y || (RMI4_CORE=m && VIDEO_V4L2=m)
select VIDEOBUF2_VMALLOC
help
Say Y here if you want to add support for RMI4 function 54
--
2.9.0
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] usb: gadget: uvc: add V4L2 dependency
Building the UVC gadget into the kernel fails to build when CONFIG_VIDEO_V4L2 is a loadable module: drivers/usb/gadget/function/usb_f_uvc.o: In function `uvc_function_ep0_complete': uvc_configfs.c:(.text.uvc_function_ep0_complete+0x84): undefined reference to `v4l2_event_queue' drivers/usb/gadget/function/usb_f_uvc.o: In function `uvc_function_disable': uvc_configfs.c:(.text.uvc_function_disable+0x34): undefined reference to `v4l2_event_queue' Adding a dependency in USB_CONFIGFS_F_UVC (which is a bool symbol) make the 'select USB_F_UVC' statement turn the USB_F_UVC into 'm' whenever CONFIG_VIDEO_V4L2=m too, avoiding the link failure. Signed-off-by: Arnd Bergmann --- drivers/usb/gadget/Kconfig | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/usb/gadget/Kconfig b/drivers/usb/gadget/Kconfig index 2ea3fc3c41b9..8ad203296079 100644 --- a/drivers/usb/gadget/Kconfig +++ b/drivers/usb/gadget/Kconfig @@ -420,6 +420,7 @@ config USB_CONFIGFS_F_HID config USB_CONFIGFS_F_UVC bool "USB Webcam function" depends on USB_CONFIGFS + depends on VIDEO_V4L2 depends on VIDEO_DEV select VIDEOBUF2_VMALLOC select USB_F_UVC -- 2.9.0 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/2] [media] Input: atmel_mxt: disallow impossible configuration
The newnly added debug mode for the atmel_mxt_ts driver relies on
the v4l2 interface and vb2_vmalloc, but those might be configured
as loadable modules when the driver itself is built-in, resulting
in a link failure:
drivers/input/touchscreen/atmel_mxt_ts.o: In function `mxt_vidioc_querycap':
atmel_mxt_ts.c:(.text.mxt_vidioc_querycap+0x10): undefined reference to
`video_devdata'
drivers/input/touchscreen/atmel_mxt_ts.o: In function `mxt_buffer_queue':
atmel_mxt_ts.c:(.text.mxt_buffer_queue+0x20): undefined reference to
`vb2_plane_vaddr'
atmel_mxt_ts.c:(.text.mxt_buffer_queue+0x164): undefined reference to
`vb2_buffer_done'
drivers/input/touchscreen/atmel_mxt_ts.o: In function `mxt_free_object_table':
atmel_mxt_ts.c:(.text.mxt_free_object_table+0x18): undefined reference to
`video_unregister_device'
atmel_mxt_ts.c:(.text.mxt_free_object_table+0x20): undefined reference to
`v4l2_device_unregister'
The best workaround I could come up with is to disallow the debug
mode unless it's actually possible to call it.
Signed-off-by: Arnd Bergmann
Fixes: ecfdd7e2660e ("[media] Input: atmel_mxt_ts - output diagnostic debug via
V4L2 device")
---
drivers/input/touchscreen/Kconfig | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/input/touchscreen/Kconfig
b/drivers/input/touchscreen/Kconfig
index fce1e41ffe8b..ccf933969587 100644
--- a/drivers/input/touchscreen/Kconfig
+++ b/drivers/input/touchscreen/Kconfig
@@ -117,7 +117,8 @@ config TOUCHSCREEN_ATMEL_MXT
config TOUCHSCREEN_ATMEL_MXT_T37
bool "Support T37 Diagnostic Data"
- depends on TOUCHSCREEN_ATMEL_MXT && VIDEO_V4L2
+ depends on TOUCHSCREEN_ATMEL_MXT
+ depends on VIDEO_V4L2=y || (TOUCHSCREEN_ATMEL_MXT=m && VIDEO_V4L2=m)
select VIDEOBUF2_VMALLOC
help
Say Y here if you want support to output data from the T37
--
2.9.0
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/2] [media] ad5820: use __maybe_unused for PM functions
The new ad5820 driver uses #ifdef to hide the suspend/resume functions,
but gets it wrong when CONFIG_PM_SLEEP is disabled:
drivers/media/i2c/ad5820.c:286:12: error: 'ad5820_resume' defined but not used
[-Werror=unused-function]
drivers/media/i2c/ad5820.c:274:12: error: 'ad5820_suspend' defined but not used
[-Werror=unused-function]
This replaces the #ifdef with a __maybe_unused annotation that is
simpler and harder to get wrong, avoiding the warning.
Signed-off-by: Arnd Bergmann
Fixes: bee3d5115611 ("[media] ad5820: Add driver for auto-focus coil")
---
drivers/media/i2c/ad5820.c | 13 ++---
1 file changed, 2 insertions(+), 11 deletions(-)
diff --git a/drivers/media/i2c/ad5820.c b/drivers/media/i2c/ad5820.c
index fd4c5f67163d..beab2f381b81 100644
--- a/drivers/media/i2c/ad5820.c
+++ b/drivers/media/i2c/ad5820.c
@@ -269,9 +269,7 @@ static const struct v4l2_subdev_internal_ops
ad5820_internal_ops = {
/*
* I2C driver
*/
-#ifdef CONFIG_PM
-
-static int ad5820_suspend(struct device *dev)
+static int __maybe_unused ad5820_suspend(struct device *dev)
{
struct i2c_client *client = container_of(dev, struct i2c_client, dev);
struct v4l2_subdev *subdev = i2c_get_clientdata(client);
@@ -283,7 +281,7 @@ static int ad5820_suspend(struct device *dev)
return ad5820_power_off(coil, false);
}
-static int ad5820_resume(struct device *dev)
+static int __maybe_unused ad5820_resume(struct device *dev)
{
struct i2c_client *client = container_of(dev, struct i2c_client, dev);
struct v4l2_subdev *subdev = i2c_get_clientdata(client);
@@ -295,13 +293,6 @@ static int ad5820_resume(struct device *dev)
return ad5820_power_on(coil, true);
}
-#else
-
-#define ad5820_suspend NULL
-#define ad5820_resume NULL
-
-#endif /* CONFIG_PM */
-
static int ad5820_probe(struct i2c_client *client,
const struct i2c_device_id *devid)
{
--
2.9.0
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/2] [media] atmel-isc: mark PM functions as __maybe_unused
The newly added atmel-isc driver uses SET_RUNTIME_PM_OPS() to
refer to its suspend/resume functions, causing a warning when
CONFIG_PM is not set:
media/platform/atmel/atmel-isc.c:1477:12: error: 'isc_runtime_resume' defined
but not used [-Werror=unused-function]
media/platform/atmel/atmel-isc.c:1467:12: error: 'isc_runtime_suspend' defined
but not used [-Werror=unused-function]
This adds __maybe_unused annotations to avoid the warning without
adding an error-prone #ifdef around it.
Signed-off-by: Arnd Bergmann
---
drivers/media/platform/atmel/atmel-isc.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/media/platform/atmel/atmel-isc.c
b/drivers/media/platform/atmel/atmel-isc.c
index db6773de92f0..a9ab7ae89f04 100644
--- a/drivers/media/platform/atmel/atmel-isc.c
+++ b/drivers/media/platform/atmel/atmel-isc.c
@@ -1464,7 +1464,7 @@ static int atmel_isc_remove(struct platform_device *pdev)
return 0;
}
-static int isc_runtime_suspend(struct device *dev)
+static int __maybe_unused isc_runtime_suspend(struct device *dev)
{
struct isc_device *isc = dev_get_drvdata(dev);
@@ -1474,7 +1474,7 @@ static int isc_runtime_suspend(struct device *dev)
return 0;
}
-static int isc_runtime_resume(struct device *dev)
+static int __maybe_unused isc_runtime_resume(struct device *dev)
{
struct isc_device *isc = dev_get_drvdata(dev);
int ret;
--
2.9.0
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 00/26] constify local structures
On Mon, Sep 12, 2016 at 3:43 PM, Felipe Balbi
wrote:
> Jarkko Sakkinen writes:
>> On Mon, Sep 12, 2016 at 10:54:07AM +0200, Julia Lawall wrote:
>>> On Sun, 11 Sep 2016, Jarkko Sakkinen wrote:
>>> > On Sun, Sep 11, 2016 at 03:05:42PM +0200, Julia Lawall wrote:
>>> > > Constify local structures.
>>> > >
>>> > > The semantic patch that makes this change is as follows:
>>> > > (http://coccinelle.lip6.fr/)
>>> >
>>> > Just my two cents but:
>>> >
>>> > 1. You *can* use a static analysis too to find bugs or other issues.
>>> > 2. However, you should manually do the commits and proper commit
>>> >messages to subsystems based on your findings. And I generally think
>>> >that if one contributes code one should also at least smoke test
>>> > changes
>>> >somehow.
>>> >
>>> > I don't know if I'm alone with my opinion. I just think that one should
>>> > also do the analysis part and not blindly create and submit patches.
>>>
>>> All of the patches are compile tested. And the individual patches are
>>
>> Compile-testing is not testing. If you are not able to test a commit,
>> you should explain why.
>
> Dude, Julia has been doing semantic patching for years already and
> nobody has raised any concerns so far. There's already an expectation
> that Coccinelle *works* and Julia's sematic patches are sound.
+1
> Besides, adding 'const' is something that causes virtually no functional
> changes to the point that build-testing is really all you need. Any
> problems caused by adding 'const' to a definition will be seen by build
> errors or warnings.
Unfortunately in this particular case they could lead to failures that can only
be detected at runtime, when failing o write to a read-only piece of memory,
due to the casting away of the constness of the pointers later.
Fortunately this was detected during code review (doh...).
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 00/26] constify local structures
On Mon, 12 Sep 2016, Felipe Balbi wrote: > > Hi, > > Jarkko Sakkinen writes: > > On Mon, Sep 12, 2016 at 10:54:07AM +0200, Julia Lawall wrote: > >> > >> > >> On Sun, 11 Sep 2016, Jarkko Sakkinen wrote: > >> > >> > On Sun, Sep 11, 2016 at 03:05:42PM +0200, Julia Lawall wrote: > >> > > Constify local structures. > >> > > > >> > > The semantic patch that makes this change is as follows: > >> > > (http://coccinelle.lip6.fr/) > >> > > >> > Just my two cents but: > >> > > >> > 1. You *can* use a static analysis too to find bugs or other issues. > >> > 2. However, you should manually do the commits and proper commit > >> >messages to subsystems based on your findings. And I generally think > >> >that if one contributes code one should also at least smoke test > >> > changes > >> >somehow. > >> > > >> > I don't know if I'm alone with my opinion. I just think that one should > >> > also do the analysis part and not blindly create and submit patches. > >> > >> All of the patches are compile tested. And the individual patches are > > > > Compile-testing is not testing. If you are not able to test a commit, > > you should explain why. > > Dude, Julia has been doing semantic patching for years already and > nobody has raised any concerns so far. There's already an expectation > that Coccinelle *works* and Julia's sematic patches are sound. > > Besides, adding 'const' is something that causes virtually no functional > changes to the point that build-testing is really all you need. Any > problems caused by adding 'const' to a definition will be seen by build > errors or warnings. > > Really, just stop with the pointless discussion and go read a bit about > Coccinelle and what semantic patches are giving you. The work done by > Julia and her peers are INRIA have measurable benefits. > > You're really making a thunderstorm in a glass of water. Thanks for the defense, but since a lot of these patches torned out to be wrong, due to an incorrect parse by Coccinelle, combined with an unpleasantly lax compiler, Jarkko does have a point that I should have looked at the patches more carefully. In any case, I have written to the maintainers relevant to the patches that turned out to be incorrect. julia -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 00/26] constify local structures
Hi, Jarkko Sakkinen writes: > On Mon, Sep 12, 2016 at 10:54:07AM +0200, Julia Lawall wrote: >> >> >> On Sun, 11 Sep 2016, Jarkko Sakkinen wrote: >> >> > On Sun, Sep 11, 2016 at 03:05:42PM +0200, Julia Lawall wrote: >> > > Constify local structures. >> > > >> > > The semantic patch that makes this change is as follows: >> > > (http://coccinelle.lip6.fr/) >> > >> > Just my two cents but: >> > >> > 1. You *can* use a static analysis too to find bugs or other issues. >> > 2. However, you should manually do the commits and proper commit >> >messages to subsystems based on your findings. And I generally think >> >that if one contributes code one should also at least smoke test changes >> >somehow. >> > >> > I don't know if I'm alone with my opinion. I just think that one should >> > also do the analysis part and not blindly create and submit patches. >> >> All of the patches are compile tested. And the individual patches are > > Compile-testing is not testing. If you are not able to test a commit, > you should explain why. Dude, Julia has been doing semantic patching for years already and nobody has raised any concerns so far. There's already an expectation that Coccinelle *works* and Julia's sematic patches are sound. Besides, adding 'const' is something that causes virtually no functional changes to the point that build-testing is really all you need. Any problems caused by adding 'const' to a definition will be seen by build errors or warnings. Really, just stop with the pointless discussion and go read a bit about Coccinelle and what semantic patches are giving you. The work done by Julia and her peers are INRIA have measurable benefits. You're really making a thunderstorm in a glass of water. -- balbi signature.asc Description: PGP signature
Re: [PATCH v2 6/7] Documentation: bindings: add documentation for ir-spi device driver
On Fri, Sep 02, 2016 at 02:33:20PM +0900, Andi Shyti wrote: > Hi Rob, > > > > Document the ir-spi driver's binding which is a IR led driven > > > through the SPI line. > > > > > > Signed-off-by: Andi Shyti > > > --- > > > Documentation/devicetree/bindings/media/spi-ir.txt | 26 > > > ++ > > > 1 file changed, 26 insertions(+) > > > create mode 100644 Documentation/devicetree/bindings/media/spi-ir.txt > > > > > > diff --git a/Documentation/devicetree/bindings/media/spi-ir.txt > > > b/Documentation/devicetree/bindings/media/spi-ir.txt > > > new file mode 100644 > > > index 000..85cb21b > > > --- /dev/null > > > +++ b/Documentation/devicetree/bindings/media/spi-ir.txt > > > > Move this to bindings/leds, and CC the leds maintainers. > > More than an LED this is the driver of a remote controller, the > driver itself is under drivers/media/rc/. The hardware is just an LED though and bindings describe h/w. What you are using it for doesn't really matter. You only need to know it's an IR led. > Besides all the transmitters have an LED but still they are media > devices. This is a bit special because it's so simple that the > only hardware left is the LED itself, but still it's a media remote > controller. > > > > @@ -0,0 +1,26 @@ > > > +Device tree bindings for IR LED connected through SPI bus which is used > > > as > > > +remote controller. > > > + > > > +The IR LED switch is connected to the MOSI line of the SPI device and > > > the data > > > +are delivered thourgh that. > > > + > > > +Required properties: > > > + - compatible: should be "ir-spi". > > > > Really this is just an LED connected to a SPI, so maybe this should > > just be "spi-led". If being more specific is helpful, then I'm all for > > that, but perhaps spi-ir-led. (Trying to be consistent in naming with > > gpio-leds). > > As I mentioned above, all transmitters have an LED, but they do > not have the 'led' name. "ir-spi" is coherent with the device > driver name and the driver name is coherent with the media/rc > driver's naming. The driver name is irrelevant to the binding. [...] > If it's OK for you, I would keep the name and documentation path > and fix the rest. Please let me know if I'm missing something :) It's not OK for me. Rob -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 00/26] constify local structures
On Mon, 12 Sep 2016, Jarkko Sakkinen wrote: > On Mon, Sep 12, 2016 at 10:54:07AM +0200, Julia Lawall wrote: > > > > > > On Sun, 11 Sep 2016, Jarkko Sakkinen wrote: > > > > > On Sun, Sep 11, 2016 at 03:05:42PM +0200, Julia Lawall wrote: > > > > Constify local structures. > > > > > > > > The semantic patch that makes this change is as follows: > > > > (http://coccinelle.lip6.fr/) > > > > > > Just my two cents but: > > > > > > 1. You *can* use a static analysis too to find bugs or other issues. > > > 2. However, you should manually do the commits and proper commit > > >messages to subsystems based on your findings. And I generally think > > >that if one contributes code one should also at least smoke test > > > changes > > >somehow. > > > > > > I don't know if I'm alone with my opinion. I just think that one should > > > also do the analysis part and not blindly create and submit patches. > > > > All of the patches are compile tested. And the individual patches are > > Compile-testing is not testing. If you are not able to test a commit, > you should explain why. > > > submitted to the relevant maintainers. The individual commit messages > > give a more detailed explanation of the strategy used to decide that the > > structure was constifiable. It seemed redundant to put that in the cover > > letter, which will not be committed anyway. > > I don't mean to be harsh but I do not care about your thought process > *that much* when I review a commit (sometimes it might make sense to > explain that but it depends on the context). > > I mostly only care why a particular change makes sense for this > particular subsystem. The report given by a static analysis tool can > be a starting point for making a commit but it's not sufficient. > Based on the report you should look subsystems as individuals. OK, thanks for the feedback. julia -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 00/26] constify local structures
On Mon, Sep 12, 2016 at 10:54:07AM +0200, Julia Lawall wrote: > > > On Sun, 11 Sep 2016, Jarkko Sakkinen wrote: > > > On Sun, Sep 11, 2016 at 03:05:42PM +0200, Julia Lawall wrote: > > > Constify local structures. > > > > > > The semantic patch that makes this change is as follows: > > > (http://coccinelle.lip6.fr/) > > > > Just my two cents but: > > > > 1. You *can* use a static analysis too to find bugs or other issues. > > 2. However, you should manually do the commits and proper commit > >messages to subsystems based on your findings. And I generally think > >that if one contributes code one should also at least smoke test changes > >somehow. > > > > I don't know if I'm alone with my opinion. I just think that one should > > also do the analysis part and not blindly create and submit patches. > > All of the patches are compile tested. And the individual patches are Compile-testing is not testing. If you are not able to test a commit, you should explain why. > submitted to the relevant maintainers. The individual commit messages > give a more detailed explanation of the strategy used to decide that the > structure was constifiable. It seemed redundant to put that in the cover > letter, which will not be committed anyway. I don't mean to be harsh but I do not care about your thought process *that much* when I review a commit (sometimes it might make sense to explain that but it depends on the context). I mostly only care why a particular change makes sense for this particular subsystem. The report given by a static analysis tool can be a starting point for making a commit but it's not sufficient. Based on the report you should look subsystems as individuals. > julia /Jarkko -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] [media] MAINTAINERS: add vimc entry
Signed-off-by: Helen Koike --- MAINTAINERS | 8 1 file changed, 8 insertions(+) diff --git a/MAINTAINERS b/MAINTAINERS index 0a16a82..43e0eb4 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -12540,6 +12540,14 @@ W: https://linuxtv.org S: Maintained F: drivers/media/platform/vivid/* +VIMC VIRTUAL MEDIA CONTROLLER DRIVER +M: Helen Koike +L: linux-media@vger.kernel.org +T: git git://linuxtv.org/media_tree.git +W: https://linuxtv.org +S: Maintained +F: drivers/media/platform/vimc/* + VLAN (802.1Q) M: Patrick McHardy L: net...@vger.kernel.org -- 2.7.4 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V2] [media] staging/media/cec: fix coding style error
On Sun, Sep 11, 2016 at 08:42:21PM +0300, Antti Palosaari wrote:
> On 09/11/2016 07:07 PM, Richard wrote:
> > Greetings Linux Kernel Developers,
> >
> > This is Task 10 of the Eudyptula Challenge, i fix few line over 80
> > characters, hope you will accept this pacth.
> >
> > /Richard
> >
> > For the eudyptula challenge (http://eudyptula-challenge.org/).
> > Simple style fix for few line over 80 characters
>
> > - if (!is_broadcast && !is_reply && !adap->follower_cnt &&
> > + if (is_directed && !is_reply && !adap->follower_cnt &&
>
> !!
> Antti
>
> --
> http://palosaari.fi/
sorry, for my mistake, i made another patch and double check it this
time.
Signed-off-by: Richard
---
drivers/staging/media/cec/cec-adap.c | 18 --
drivers/staging/media/cec/cec-api.c | 6 --
drivers/staging/media/cec/cec-core.c | 9 ++---
drivers/staging/media/cec/cec-priv.h | 3 ++-
4 files changed, 24 insertions(+), 12 deletions(-)
diff --git a/drivers/staging/media/cec/cec-adap.c
b/drivers/staging/media/cec/cec-adap.c
index 611e07b..8aedd22 100644
--- a/drivers/staging/media/cec/cec-adap.c
+++ b/drivers/staging/media/cec/cec-adap.c
@@ -1,7 +1,8 @@
/*
* cec-adap.c - HDMI Consumer Electronics Control framework - CEC adapter
*
- * Copyright 2016 Cisco Systems, Inc. and/or its affiliates. All rights
reserved.
+ * Copyright 2016 Cisco Systems, Inc. and/or its affiliates.
+ * All rights reserved.
*
* This program is free software; you may redistribute it and/or modify
* it under the terms of the GNU General Public License as published by
@@ -64,7 +65,8 @@ static int cec_log_addr2idx(const struct cec_adapter *adap,
u8 log_addr)
return -1;
}
-static unsigned int cec_log_addr2dev(const struct cec_adapter *adap, u8
log_addr)
+static unsigned int cec_log_addr2dev(const struct cec_adapter *adap,
+u8 log_addr)
{
int i = cec_log_addr2idx(adap, log_addr);
@@ -330,9 +332,11 @@ int cec_thread_func(void *_adap)
* see if the adapter is disabled in which case the
* transmit should be canceled.
*/
- err =
wait_event_interruptible_timeout(adap->kthread_waitq,
+ err = wait_event_interruptible_timeout(
+ adap->kthread_waitq,
kthread_should_stop() ||
- (!adap->is_configured && !adap->is_configuring)
||
+ (!adap->is_configured &&
+!adap->is_configuring) ||
(!adap->transmitting &&
!list_empty(&adap->transmit_queue)),
msecs_to_jiffies(CEC_XFER_TIMEOUT_MS));
@@ -1534,13 +1538,15 @@ static int cec_receive_notify(struct cec_adapter *adap,
struct cec_msg *msg,
/* Do nothing for CEC switches using addr 15 */
if (devtype == CEC_OP_PRIM_DEVTYPE_SWITCH && dest_laddr == 15)
return 0;
- cec_msg_report_physical_addr(&tx_cec_msg, adap->phys_addr,
devtype);
+ cec_msg_report_physical_addr(&tx_cec_msg, adap->phys_addr,
+devtype);
return cec_transmit_msg(adap, &tx_cec_msg, false);
case CEC_MSG_GIVE_DEVICE_VENDOR_ID:
if (adap->log_addrs.vendor_id == CEC_VENDOR_ID_NONE)
return cec_feature_abort(adap, msg);
- cec_msg_device_vendor_id(&tx_cec_msg,
adap->log_addrs.vendor_id);
+ cec_msg_device_vendor_id(&tx_cec_msg,
+adap->log_addrs.vendor_id);
return cec_transmit_msg(adap, &tx_cec_msg, false);
case CEC_MSG_ABORT:
diff --git a/drivers/staging/media/cec/cec-api.c
b/drivers/staging/media/cec/cec-api.c
index e274e2f..c14a0c1 100644
--- a/drivers/staging/media/cec/cec-api.c
+++ b/drivers/staging/media/cec/cec-api.c
@@ -1,7 +1,8 @@
/*
* cec-api.c - HDMI Consumer Electronics Control framework - API
*
- * Copyright 2016 Cisco Systems, Inc. and/or its affiliates. All rights
reserved.
+ * Copyright 2016 Cisco Systems, Inc. and/or its affiliates.
+ * All rights reserved.
*
* This program is free software; you may redistribute it and/or modify
* it under the terms of the GNU General Public License as published by
@@ -548,7 +549,8 @@ static int cec_release(struct inode *inode, struct file
*filp)
mutex_lock(&adap->lock);
while (!list_empty(&fh->xfer_list)) {
struct cec_data *data =
- list_first_entry(&fh->xfer_list, struct cec_data,
xfer_list);
+ list_first_entry(&fh->xfer_list, struct cec_data,
+xfer_list);
data->blocking = false;
data->fh = NULL;
diff
Re: [PATCH 00/26] constify local structures
On Sun, 11 Sep 2016, Jarkko Sakkinen wrote:
> On Sun, Sep 11, 2016 at 03:05:42PM +0200, Julia Lawall wrote:
> > Constify local structures.
> >
> > The semantic patch that makes this change is as follows:
> > (http://coccinelle.lip6.fr/)
>
> Just my two cents but:
>
> 1. You *can* use a static analysis too to find bugs or other issues.
> 2. However, you should manually do the commits and proper commit
>messages to subsystems based on your findings. And I generally think
>that if one contributes code one should also at least smoke test changes
>somehow.
>
> I don't know if I'm alone with my opinion. I just think that one should
> also do the analysis part and not blindly create and submit patches.
All of the patches are compile tested. And the individual patches are
submitted to the relevant maintainers. The individual commit messages
give a more detailed explanation of the strategy used to decide that the
structure was constifiable. It seemed redundant to put that in the cover
letter, which will not be committed anyway.
julia
>
> Anyway, I'll apply the TPM change at some point. As I said they were
> for better. Thanks.
>
> /Jarkko
>
> > //
> > // The first rule ignores some cases that posed problems
> > @r disable optional_qualifier@
> > identifier s != {peri_clk_data,threshold_attr,tracer_flags,tracer};
> > identifier i != {s5k5baf_cis_rect,smtcfb_fix};
> > position p;
> > @@
> > static struct s i@p = { ... };
> >
> > @lstruct@
> > identifier r.s;
> > @@
> > struct s { ... };
> >
> > @used depends on lstruct@
> > identifier r.i;
> > @@
> > i
> >
> > @bad1@
> > expression e;
> > identifier r.i;
> > assignment operator a;
> > @@
> > (<+...i...+>) a e
> >
> > @bad2@
> > identifier r.i;
> > @@
> > &(<+...i...+>)
> >
> > @bad3@
> > identifier r.i;
> > declarer d;
> > @@
> > d(...,<+...i...+>,...);
> >
> > @bad4@
> > identifier r.i;
> > type T;
> > T[] e;
> > identifier f;
> > position p;
> > @@
> >
> > f@p(...,
> > (
> > (<+...i...+>)
> > &
> > e
> > )
> > ,...)
> >
> > @bad4a@
> > identifier r.i;
> > type T;
> > T *e;
> > identifier f;
> > position p;
> > @@
> >
> > f@p(...,
> > (
> > (<+...i...+>)
> > &
> > e
> > )
> > ,...)
> >
> > @ok5@
> > expression *e;
> > identifier r.i;
> > position p;
> > @@
> > e =@p i
> >
> > @bad5@
> > expression *e;
> > identifier r.i;
> > position p != ok5.p;
> > @@
> > e =@p (<+...i...+>)
> >
> > @rr depends on used && !bad1 && !bad2 && !bad3 && !bad4 && !bad4a && !bad5@
> > identifier s,r.i;
> > position r.p;
> > @@
> >
> > static
> > +const
> > struct s i@p = { ... };
> >
> > @depends on used && !bad1 && !bad2 && !bad3 && !bad4 && !bad4a && !bad5
> > disable optional_qualifier@
> > identifier rr.s,r.i;
> > @@
> >
> > static
> > +const
> > struct s i;
> > //
> >
> > ---
> >
> > drivers/acpi/acpi_apd.c |8 +++---
> > drivers/char/tpm/tpm-interface.c | 10
> > drivers/char/tpm/tpm-sysfs.c |2 -
> > drivers/cpufreq/intel_pstate.c |8 +++---
> > drivers/infiniband/hw/i40iw/i40iw_uk.c |6 ++---
> > drivers/media/i2c/tvp514x.c |2 -
> > drivers/media/pci/ddbridge/ddbridge-core.c | 18 +++
> > drivers/media/pci/ngene/ngene-cards.c| 14 ++--
> > drivers/media/pci/smipcie/smipcie-main.c |8 +++---
> > drivers/misc/sgi-xp/xpc_uv.c |2 -
> > drivers/net/arcnet/com20020-pci.c| 10
> > drivers/net/can/c_can/c_can_pci.c|4 +--
> > drivers/net/can/sja1000/plx_pci.c| 20
> > -
> > drivers/net/ethernet/mellanox/mlx4/main.c|4 +--
> > drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c |2 -
> > drivers/net/ethernet/renesas/sh_eth.c| 14 ++--
> > drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c |2 -
> > drivers/net/wireless/ath/dfs_pattern_detector.c |2 -
> > drivers/net/wireless/intel/iwlegacy/3945.c |4 +--
> > drivers/net/wireless/realtek/rtlwifi/rtl8188ee/sw.c |2 -
> > drivers/net/wireless/realtek/rtlwifi/rtl8192ce/sw.c |2 -
> > drivers/net/wireless/realtek/rtlwifi/rtl8192de/sw.c |2 -
> > drivers/net/wireless/realtek/rtlwifi/rtl8192ee/sw.c |2 -
> > drivers/net/wireless/realtek/rtlwifi/rtl8192se/sw.c |2 -
> > drivers/net/wireless/realtek/rtlwifi/rtl8723ae/sw.c |2 -
> > drivers/net/wireless/realtek/rtlwifi/rtl8723be/sw.c |2 -
> > drivers/net/wireless/realtek/rtlwifi/rtl8821ae/sw.c |2 -
> > drivers/platform/chrome/chromeos_laptop.c| 22
> > +--
> > drivers/platform/x86/intel_scu_ipc.c |6 ++---
> > drivers/platform/x86/intel_telemetry_debugfs.c |2 -
> > drivers/scsi/esas2r/esas2r_flash.c |2 -
> > drive
Re: musb: isoc pkt loss with pwc
2016-09-12 6:28 GMT+03:00 Bin Liu :
> Hi,
>
> On Tue, Aug 30, 2016 at 11:44:33PM +0300, Matwey V. Kornilov wrote:
>> 2016-08-30 21:30 GMT+03:00 Bin Liu :
>> > Hi,
>> >
>> > On Sun, Aug 28, 2016 at 01:13:55PM +0300, Matwey V. Kornilov wrote:
>> >> Hello Bin,
>> >>
>> >> I would like to start new thread on my issue. Let me recall where the
>> >> issue is:
>> >> There is 100% frame lost in pwc webcam driver due to lots of
>> >> zero-length packages coming from musb driver.
>> >
>> > What is the video resolution and fps?
>>
>> 640x480 YUV420 10 frames per second.
>> pwc uses proprietary compression during device-host transmission, but
>> I don't know how effective it is.
>
> The data rate for VGA YUV420 @10fps is 640x480*1.5*10 = 4.6MB/s, which
> is much higher than full-speed 12Mbps. So the video data on the bus is
> compressed, not YUV420, I believe.
>
>>
>> >
>> >> The issue is present in all kernels (including 4.8) starting from the
>> >> commit:
>> >>
>> >> f551e13529833e052f75ec628a8af7b034af20f9 ("Revert "usb: musb:
>> >> musb_host: Enable HCD_BH flag to handle urb return in bottom half"")
>> >
>> > What is the behavior without this commit?
>>
>> Without this commit all frames are being received correctly. Single
>
> Which means without this commit your camera has been working without
> issues, and this is a regression with this commit, right?
>
Right
>> one issue is a number of zero byte package at the beginning of iso
>> stream (probably during camera internal sync, I have to check how the
>> stream is started on x86 later). But they are never repeated after
>> this.
>
> I think this zero byte packet is normal. I don't know much about pwc,
> but with uvc cameras, the beginning of the stream is similar, with many
> 12-bytes packets. 12 byte is typical uvc header length, so 12-byte
> packet means zero data payload.
>
>>
>> >>
>> >> The issue is here both when DMA enabled and DMA disabled.
>> >>
>> >> Attached here is a plot. The vertical axis designates the value of
>> >> rx_count variable from function musb_host_packet_rx(). One may see
>> >> that there are only two possibilities: 0 bytes and 956 bytes.
>> >> The horizontal axis is the last three digits of the timestamp when
>> >> musb_host_packet_rx() invoked. I.e for [ 38.115379] it is 379. Given
>> >> that my webcam is USB 1.1 and base time is 1 ms, then all frames
>> >> should be grouped close to some single value. (Repeating package
>> >> receive event every 1 ms won't change last tree digits considerably)
>> >> One may see that it is not true, in practice there are two groups. And
>> >> receive time strongly correlates with the package size. Packages
>> >> received near round ms are 956 bytes long, packages received near 400
>> >> us are 0 bytes long.
>> >
>> > When the host IN packet passed the deadline, the device drops the video
>> > data, so device only sends 0 byte packet (or 12 bytes which is only the
>> > uvc header without data payload).
>>
>> Doesn't it mean that free part of the frame, i.e (1 msec - (t_IN -
>> t_SOF)) is not enough to transmit 956 bytes in device firmware
>> opinion?
>>
>> >
>> >>
>> >> I don't know how exactly SOF and IN are synchronized in musb, could
>> >> someone give a hint? But from what I see the time difference between
>> >> subsequent IN package requests is sometimes more than 1 ms due to
>> >> heavy urb->complete() callback. After such events only zero length
>> >
>> > Why musb delayed the IN packets, it needs to be investigated. I will
>> > start to look at this webcam issue with musb soon, after I cleaned up
>> > the musb patches queued recently. I will update once I have progress in
>> > my investigation.
>>
>> The last package in URB has different code path.
>> IN after the last package of URB is not requested in musb_host_packet_rx().
>> Instead, IN request is performed in the end of musb_advance_schedule()
>> by musb_start_urb().
>
> I am seeing the samilar issue with my Logitech pro9000 camera, and I
> have been looking at it whenever I got some time.
>
> I believe the IN after the last packet is coming from a new URB, that is
> why it is performed by musb_start_urb().
>
>> musb_advance_schedule() has the following code:
>>
>> qh->is_ready = 0;
>> musb_giveback(musb, urb, status);
>> qh->is_ready = ready;
>>
>> Which can be executed pretty long if urb->complete() handler is not
>> postphoned by HCD_BH.
>> In my case, it takes about 300 us for pwc urb->complete() to run.
>
> My understanding so far is that when the current RX URB is completed in
> musb host driver, musb_giveback() is called which triggers
> urb->complete(), the uvc driver (pwc driver in your case) parses the
> packets, to know if further video data is needed. If so, a new URB is
> generated, so IN request is performed again.
Not exactly. pwc_isoc_init() submits three URBs in a row. So, when an
URB is finished there are always two more URBs pending. No need to
wait a decision from urb->complete().
>
>> Pr
[PATCH v2] [media] vivid: support for contiguous DMA buffers
It allows to simulate the behavior of hardware with such limitations or
to connect vivid to real hardware with such limitations.
Add the "allocators" module parameter option to let vivid use the
dma-contig instead of vmalloc.
Signed-off-by: Philipp Zabel
Signed-off-by: Hans Verkuil
Signed-off-by: Vincent Abriou
Cc: Philipp Zabel
Cc: Hans Verkuil
---
Documentation/media/v4l-drivers/vivid.rst | 8
drivers/media/platform/vivid/Kconfig | 2 ++
drivers/media/platform/vivid/vivid-core.c | 32 ++-
3 files changed, 37 insertions(+), 5 deletions(-)
diff --git a/Documentation/media/v4l-drivers/vivid.rst
b/Documentation/media/v4l-drivers/vivid.rst
index c8cf371..3e44b22 100644
--- a/Documentation/media/v4l-drivers/vivid.rst
+++ b/Documentation/media/v4l-drivers/vivid.rst
@@ -263,6 +263,14 @@ all configurable using the following module options:
removed. Unless overridden by ccs_cap_mode and/or ccs_out_mode the
will default to enabling crop, compose and scaling.
+- allocators:
+
+ memory allocator selection, default is 0. It specifies the way buffers
+ will be allocated.
+
+ - 0: vmalloc
+ - 1: dma-contig
+
Taken together, all these module options allow you to precisely customize
the driver behavior and test your application with all sorts of permutations.
It is also very suitable to emulate hardware that is not yet available, e.g.
diff --git a/drivers/media/platform/vivid/Kconfig
b/drivers/media/platform/vivid/Kconfig
index 8e6918c..2e238a1 100644
--- a/drivers/media/platform/vivid/Kconfig
+++ b/drivers/media/platform/vivid/Kconfig
@@ -1,6 +1,7 @@
config VIDEO_VIVID
tristate "Virtual Video Test Driver"
depends on VIDEO_DEV && VIDEO_V4L2 && !SPARC32 && !SPARC64 && FB
+ depends on HAS_DMA
select FONT_SUPPORT
select FONT_8x16
select FB_CFB_FILLRECT
@@ -8,6 +9,7 @@ config VIDEO_VIVID
select FB_CFB_IMAGEBLIT
select MEDIA_CEC_EDID
select VIDEOBUF2_VMALLOC
+ select VIDEOBUF2_DMA_CONTIG
select VIDEO_V4L2_TPG
default n
---help---
diff --git a/drivers/media/platform/vivid/vivid-core.c
b/drivers/media/platform/vivid/vivid-core.c
index 741460a..02e1909 100644
--- a/drivers/media/platform/vivid/vivid-core.c
+++ b/drivers/media/platform/vivid/vivid-core.c
@@ -30,6 +30,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -151,6 +152,12 @@ static bool no_error_inj;
module_param(no_error_inj, bool, 0444);
MODULE_PARM_DESC(no_error_inj, " if set disable the error injecting controls");
+static unsigned int allocators[VIVID_MAX_DEVS] = { [0 ... (VIVID_MAX_DEVS -
1)] = 0 };
+module_param_array(allocators, uint, NULL, 0444);
+MODULE_PARM_DESC(allocators, " memory allocator selection, default is 0.\n"
+"\t\t0 == vmalloc\n"
+"\t\t1 == dma-contig");
+
static struct vivid_dev *vivid_devs[VIVID_MAX_DEVS];
const struct v4l2_rect vivid_min_rect = {
@@ -636,6 +643,10 @@ static int vivid_create_instance(struct platform_device
*pdev, int inst)
{
static const struct v4l2_dv_timings def_dv_timings =
V4L2_DV_BT_CEA_1280X720P60;
+ static const struct vb2_mem_ops * const vivid_mem_ops[2] = {
+ &vb2_vmalloc_memops,
+ &vb2_dma_contig_memops,
+ };
unsigned in_type_counter[4] = { 0, 0, 0, 0 };
unsigned out_type_counter[4] = { 0, 0, 0, 0 };
int ccs_cap = ccs_cap_mode[inst];
@@ -646,6 +657,7 @@ static int vivid_create_instance(struct platform_device
*pdev, int inst)
struct video_device *vfd;
struct vb2_queue *q;
unsigned node_type = node_types[inst];
+ unsigned int allocator = allocators[inst];
v4l2_std_id tvnorms_cap = 0, tvnorms_out = 0;
int ret;
int i;
@@ -1036,6 +1048,11 @@ static int vivid_create_instance(struct platform_device
*pdev, int inst)
if (!dev->cec_workqueue)
goto unreg_dev;
+ if (allocator == 1)
+ dma_coerce_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32));
+ else if (allocator >= ARRAY_SIZE(vivid_mem_ops))
+ allocator = 0;
+
/* start creating the vb2 queues */
if (dev->has_vid_cap) {
/* initialize vid_cap queue */
@@ -1046,10 +1063,11 @@ static int vivid_create_instance(struct platform_device
*pdev, int inst)
q->drv_priv = dev;
q->buf_struct_size = sizeof(struct vivid_buffer);
q->ops = &vivid_vid_cap_qops;
- q->mem_ops = &vb2_vmalloc_memops;
+ q->mem_ops = vivid_mem_ops[allocator];
q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
q->min_buffers_needed = 2;
q->lock = &dev->mutex;
+ q->dev = dev->v4l2_dev.dev;
[PATCH] [media] pulse8-cec: avoid uninitialized data use
Building with -Wmaybe-uninitialized reveals the use on an uninitialized
variable containing the physical address of the device whenever
firmware before version 2 is used:
drivers/staging/media/pulse8-cec/pulse8-cec.c: In function 'pulse8_connect':
drivers/staging/media/pulse8-cec/pulse8-cec.c:447:2: error: 'pa' may be used
uninitialized in this function [-Werror=maybe-uninitialized]
This sets the address to CEC_PHYS_ADDR_INVALID in this case, so we don't
try to write back the uninitialized data to the device.
Signed-off-by: Arnd Bergmann
Fixes: e28a6c8b3fcc ("[media] pulse8-cec: sync configuration with adapter")
---
drivers/staging/media/pulse8-cec/pulse8-cec.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/staging/media/pulse8-cec/pulse8-cec.c
b/drivers/staging/media/pulse8-cec/pulse8-cec.c
index 1158ba9f828f..64fffc709416 100644
--- a/drivers/staging/media/pulse8-cec/pulse8-cec.c
+++ b/drivers/staging/media/pulse8-cec/pulse8-cec.c
@@ -342,8 +342,10 @@ static int pulse8_setup(struct pulse8 *pulse8, struct
serio *serio,
return err;
pulse8->vers = (data[0] << 8) | data[1];
dev_info(pulse8->dev, "Firmware version %04x\n", pulse8->vers);
- if (pulse8->vers < 2)
+ if (pulse8->vers < 2) {
+ *pa = CEC_PHYS_ADDR_INVALID;
return 0;
+ }
cmd[0] = MSGCODE_GET_BUILDDATE;
err = pulse8_send_and_wait(pulse8, cmd, 1, cmd[0], 4);
--
2.9.0
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] [media] atmel-isc: set the format on the first open
Set the current format on the first open.
Signed-off-by: Songjun Wu
---
drivers/media/platform/atmel/atmel-isc.c | 30 ++
1 file changed, 18 insertions(+), 12 deletions(-)
diff --git a/drivers/media/platform/atmel/atmel-isc.c
b/drivers/media/platform/atmel/atmel-isc.c
index db6773d..ed8050d 100644
--- a/drivers/media/platform/atmel/atmel-isc.c
+++ b/drivers/media/platform/atmel/atmel-isc.c
@@ -924,10 +924,16 @@ static int isc_open(struct file *file)
goto unlock;
ret = v4l2_subdev_call(sd, core, s_power, 1);
- if (ret < 0 && ret != -ENOIOCTLCMD)
+ if (ret < 0 && ret != -ENOIOCTLCMD) {
v4l2_fh_release(file);
- else
- ret = 0;
+ goto unlock;
+ }
+
+ ret = isc_set_fmt(isc, &isc->fmt);
+ if (ret) {
+ v4l2_subdev_call(sd, core, s_power, 0);
+ v4l2_fh_release(file);
+ }
unlock:
mutex_unlock(&isc->lock);
@@ -1118,8 +1124,16 @@ static int isc_set_default_fmt(struct isc_device *isc)
.pixelformat= isc->user_formats[0]->fourcc,
},
};
+ int ret;
- return isc_set_fmt(isc, &f);
+ ret = isc_try_fmt(isc, &f, NULL);
+ if (ret)
+ return ret;
+
+ isc->current_fmt = isc->user_formats[0];
+ isc->fmt = f;
+
+ return 0;
}
static int isc_async_complete(struct v4l2_async_notifier *notifier)
@@ -1172,20 +1186,12 @@ static int isc_async_complete(struct
v4l2_async_notifier *notifier)
return ret;
}
- ret = v4l2_subdev_call(sd_entity->sd, core, s_power, 1);
- if (ret < 0 && ret != -ENOIOCTLCMD)
- return ret;
-
ret = isc_set_default_fmt(isc);
if (ret) {
v4l2_err(&isc->v4l2_dev, "Could not set default format\n");
return ret;
}
- ret = v4l2_subdev_call(sd_entity->sd, core, s_power, 0);
- if (ret < 0 && ret != -ENOIOCTLCMD)
- return ret;
-
/* Register video device */
strlcpy(vdev->name, ATMEL_ISC_NAME, sizeof(vdev->name));
vdev->release = video_device_release_empty;
--
2.7.4
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[GIT PULL FOR v4.9] Fix two compilation issues
Hi Mauro, These two patches fix compilation issues for 4.9. One simple warning and one fixing duplicate functions. Regards, Hans The following changes since commit 8a5a2ba86ab8fc12267fea974b9cd730ad2dee24: [media] v4l: vsp1: Add R8A7792 VSP1V support (2016-09-09 11:32:43 -0300) are available in the git repository at: git://linuxtv.org/hverkuil/media_tree.git for-v4.9d for you to fetch changes up to 2fb0feefa258953ebdb0f81931f4725fd5498c14: pulse8-cec: fix compiler warning (2016-09-11 11:00:15 +0200) Hans Verkuil (2): pxa_camera: merge soc_mediabus.c into pxa_camera.c pulse8-cec: fix compiler warning drivers/media/platform/Makefile | 2 +- drivers/media/platform/pxa_camera.c | 482 drivers/staging/media/pulse8-cec/pulse8-cec.c | 2 +- 3 files changed, 460 insertions(+), 26 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
