pvrusb2 has a new device (wintv-hvr-1955)
Just bought a wintv-hvr-1955 (sold as a wintv-hvr-1950) 160111 LF Rev B1|7 It has a slightly new usb id, 2040:7502 vs 2040:7501 I edited the kernel to have the driver as it exists for the 7501 for the 7502 (as you can imagine, it didn't work). It still has the CY7C68013A USB pieces, but I don't know how to check the tuner and demodulators (which I imagine is why this is failing to work). Here's some dmesg output of it failing to load, this is about as far I can get with my current linux hacking knowledge. [ 391.895250] usb 1-4: new high-speed USB device number 5 using ehci-pci [ 392.333922] usb 1-4: New USB device found, idVendor=2040, idProduct=7502 [ 392.333927] usb 1-4: New USB device strings: Mfr=1, Product=2, SerialNumber=3 [ 392.333929] usb 1-4: Product: WinTV [ 392.333930] usb 1-4: Manufacturer: Hauppauge [ 392.333932] usb 1-4: SerialNumber: 7300-00-F084C888 [ 392.337566] pvrusb2: Hardware description: WinTV HVR-1950 Model 751xx [ 392.373602] pvrusb2: Binding ir_rx_z8f0811_haup to i2c address 0x71. [ 392.373627] Registered IR keymap rc-hauppauge [ 392.373702] input: i2c IR (WinTV HVR-1950 Model 75 as /devices/virtual/rc/rc0/input5 [ 392.373742] rc0: i2c IR (WinTV HVR-1950 Model 75 as /devices/virtual/rc/rc0 [ 392.373744] ir-kbd-i2c: i2c IR (WinTV HVR-1950 Model 75 detected at i2c-0/0-0071/ir0 [pvrusb2_a] [ 392.373749] pvrusb2: Binding ir_tx_z8f0811_haup to i2c address 0x70. [ 392.377934] cx25840 0-0044: cx25843-24 found @ 0x88 (pvrusb2_a) [ 392.395116] pvrusb2: Attached sub-driver cx25840 [ 392.397754] tda9887 0-0042: creating new instance [ 392.397757] tda9887 0-0042: tda988[5/6/7] found [ 392.400183] tda9887 0-0042: i2c i/o error: rc == -5 (should be 4) [ 392.400186] tuner 0-0042: Tuner 74 found with type(s) Radio TV. [ 392.400202] pvrusb2: Attached sub-driver tuner [ 394.808884] cx25840 0-0044: loaded v4l-cx25840.fw firmware (16382 bytes) [ 394.970408] tveeprom 0-00a2: Hauppauge model 160111, rev B1I7, serial# 8702088 [ 394.970412] tveeprom 0-00a2: MAC address is 00:0d:fe:84:c8:88 [ 394.970413] tveeprom 0-00a2: tuner model is unknown (idx 187, type 4) [ 394.970415] tveeprom 0-00a2: TV standards NTSC(M) ATSC/DVB Digital (eeprom 0x88) [ 394.970417] tveeprom 0-00a2: audio processor is CX25843 (idx 37) [ 394.970419] tveeprom 0-00a2: decoder processor is CX25843 (idx 30) [ 394.970420] tveeprom 0-00a2: has radio, has IR receiver, has IR transmitter [ 394.970425] pvrusb2: Supported video standard(s) reported available in hardware: PAL-M/N/Nc;NTSC-M/Mj/Mk;ATSC-8VSB/16VSB [ 394.970427] pvrusb2: Initial video standard (determined by device type): NTSC-M [ 394.970442] pvrusb2: Device initialization completed successfully. [ 394.970614] pvrusb2: registered device video0 [mpeg] [ 394.970617] DVB: registering new adapter (pvrusb2-dvb) [ 397.421679] cx25840 0-0044: loaded v4l-cx25840.fw firmware (16382 bytes) [ 397.590387] tda9887 0-0042: i2c i/o error: rc == -5 (should be 4) [ 397.648404] tda9887 0-0042: i2c i/o error: rc == -5 (should be 4) [ 397.667384] tda9887 0-0042: i2c i/o error: rc == -5 (should be 4) [ 398.059485] tda9887 0-0042: i2c i/o error: rc == -5 (should be 4) [ 398.111284] tda9887 0-0042: i2c i/o error: rc == -5 (should be 4) [ 398.128856] tda9887 0-0042: i2c i/o error: rc == -5 (should be 4) [ 398.128860] cx25840 0-0044: 0x is not a valid video input! [ 398.169176] s5h1411_readreg: readreg error (ret == -5) [ 398.169179] pvrusb2: no frontend was attached! [ 398.169181] pvrusb2: unregistering DVB devices -- -- Matthew Thode (prometheanfire) signature.asc Description: OpenPGP digital signature
cron job: media_tree daily build: OK
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: Fri Jun 20 04:00:34 CEST 2014 git branch: test git hash: 1fe3a8fe494463cfe2556a25ae41a1499725c178 gcc version:i686-linux-gcc (GCC) 4.8.2 sparse version: v0.5.0-14-gf11dd94 host hardware: x86_64 host os:3.14-5.slh.5-amd64 linux-git-arm-at91: OK linux-git-arm-davinci: OK linux-git-arm-exynos: OK linux-git-arm-mx: OK linux-git-arm-omap: OK linux-git-arm-omap1: OK linux-git-arm-pxa: OK linux-git-blackfin: 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.31.14-i686: OK linux-2.6.32.27-i686: OK linux-2.6.33.7-i686: OK linux-2.6.34.7-i686: OK linux-2.6.35.9-i686: 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-i686: OK linux-3.13-i686: OK linux-3.14-i686: OK linux-3.15-i686: OK linux-3.16-rc1-i686: OK linux-2.6.31.14-x86_64: OK linux-2.6.32.27-x86_64: OK linux-2.6.33.7-x86_64: OK linux-2.6.34.7-x86_64: OK linux-2.6.35.9-x86_64: 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-x86_64: OK linux-3.13-x86_64: OK linux-3.14-x86_64: OK linux-3.15-x86_64: OK linux-3.16-rc1-x86_64: OK apps: OK spec-git: OK sparse: WARNINGS Detailed results are available here: http://www.xs4all.nl/~hverkuil/logs/Friday.log Full logs are available here: http://www.xs4all.nl/~hverkuil/logs/Friday.tar.bz2 The Media Infrastructure API from this daily build is here: http://www.xs4all.nl/~hverkuil/spec/media.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] staging: solo6x10: fix for sparse warning message
Define jpeg_dqt as static.
Signed-off-by: Anthony DeStefano
---
drivers/staging/media/solo6x10/solo6x10-jpeg.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/staging/media/solo6x10/solo6x10-jpeg.h
b/drivers/staging/media/solo6x10/solo6x10-jpeg.h
index c5218ce..9e41185 100644
--- a/drivers/staging/media/solo6x10/solo6x10-jpeg.h
+++ b/drivers/staging/media/solo6x10/solo6x10-jpeg.h
@@ -110,7 +110,7 @@ static const unsigned char jpeg_header[] = {
/* This is the byte marker for the start of the DQT */
#define DQT_START 17
#define DQT_LEN138
-const unsigned char jpeg_dqt[4][DQT_LEN] = {
+static const unsigned char jpeg_dqt[4][DQT_LEN] = {
{
0xff, 0xdb, 0x00, 0x43, 0x00,
0x08, 0x06, 0x06, 0x07, 0x06, 0x05, 0x08, 0x07,
--
2.0.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: [REPOST PATCH 1/8] fence: dma-buf cross-device synchronization (v17)
On Thu, Jun 19, 2014 at 03:39:47PM -0700, H. Peter Anvin wrote: > On 06/19/2014 01:01 PM, Greg KH wrote: > > On Thu, Jun 19, 2014 at 09:15:36PM +0200, Daniel Vetter wrote: > >> On Thu, Jun 19, 2014 at 7:00 PM, Greg KH > >> wrote: > >> + BUG_ON(f1->context != f2->context); > > > > Nice, you just crashed the kernel, making it impossible to debug or > > recover :( > > agreed, that should probably be 'if (WARN_ON(...)) return NULL;' > > (but at least I wouldn't expect to hit that under console_lock so you > should at least see the last N lines of the backtrace on the screen > ;-)) > >>> > >>> Lots of devices don't have console screens :) > >> > >> Aside: This is a pet peeve of mine and recently I've switched to > >> rejecting all patch that have a BUG_ON, period. > > > > Please do, I have been for a few years now as well for the same reasons > > you cite. > > > > I'm actually concerned about this trend. Downgrading things to WARN_ON > can allow a security bug in the kernel to continue to exist, for > example, or make the error message disappear. A BUG_ON makes any error message disappear pretty quickly :) I'm talking about foolish "ASSERT-like" BUG_ON that driver authors like to add to their code when writing it to catch things they are messing up. After the code is working, they should be removed, like this one. Don't enforce an api requirement with a kernel crash, warn and return an error which the caller should always be checking anyway. thanks, greg k-h -- 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: [REPOST PATCH 1/8] fence: dma-buf cross-device synchronization (v17)
On Thu, Jun 19, 2014 at 5:50 PM, Dave Airlie wrote:
> On 20 June 2014 04:19, Greg KH wrote:
>> On Thu, Jun 19, 2014 at 01:45:30PM -0400, Rob Clark wrote:
>>> On Thu, Jun 19, 2014 at 1:00 PM, Greg KH wrote:
>>> > On Thu, Jun 19, 2014 at 10:00:18AM -0400, Rob Clark wrote:
>>> >> On Wed, Jun 18, 2014 at 9:13 PM, Greg KH
>>> >> wrote:
>>> >> > On Wed, Jun 18, 2014 at 12:36:54PM +0200, Maarten Lankhorst wrote:
>>> >> >> +#define CREATE_TRACE_POINTS
>>> >> >> +#include
>>> >> >> +
>>> >> >> +EXPORT_TRACEPOINT_SYMBOL(fence_annotate_wait_on);
>>> >> >> +EXPORT_TRACEPOINT_SYMBOL(fence_emit);
>>> >> >
>>> >> > Are you really willing to live with these as tracepoints for forever?
>>> >> > What is the use of them in debugging? Was it just for debugging the
>>> >> > fence code, or for something else?
>>> >> >
>>> >> >> +/**
>>> >> >> + * fence_context_alloc - allocate an array of fence contexts
>>> >> >> + * @num: [in]amount of contexts to allocate
>>> >> >> + *
>>> >> >> + * This function will return the first index of the number of fences
>>> >> >> allocated.
>>> >> >> + * The fence context is used for setting fence->context to a unique
>>> >> >> number.
>>> >> >> + */
>>> >> >> +unsigned fence_context_alloc(unsigned num)
>>> >> >> +{
>>> >> >> + BUG_ON(!num);
>>> >> >> + return atomic_add_return(num, &fence_context_counter) - num;
>>> >> >> +}
>>> >> >> +EXPORT_SYMBOL(fence_context_alloc);
>>> >> >
>>> >> > EXPORT_SYMBOL_GPL()? Same goes for all of the exports in here.
>>> >> > Traditionally all of the driver core exports have been with this
>>> >> > marking, any objection to making that change here as well?
>>> >>
>>> >> tbh, I prefer EXPORT_SYMBOL().. well, I'd prefer even more if there
>>> >> wasn't even a need for EXPORT_SYMBOL_GPL(), but sadly it is a fact of
>>> >> life. We already went through this debate once with dma-buf. We
>>> >> aren't going to change $evil_vendor's mind about non-gpl modules. The
>>> >> only result will be a more flugly convoluted solution (ie. use syncpt
>>> >> EXPORT_SYMBOL() on top of fence EXPORT_SYMBOL_GPL()) just as a
>>> >> workaround, with the result that no-one benefits.
>>> >
>>> > It has been proven that using _GPL() exports have caused companies to
>>> > release their code "properly" over the years, so as these really are
>>> > Linux-only apis, please change them to be marked this way, it helps
>>> > everyone out in the end.
>>>
>>> Well, maybe that is the true in some cases. But it certainly didn't
>>> work out that way for dma-buf. And I think the end result is worse.
>>>
>>> I don't really like coming down on the side of EXPORT_SYMBOL() instead
>>> of EXPORT_SYMBOL_GPL(), but if we do use EXPORT_SYMBOL_GPL() then the
>>> result will only be creative workarounds using the _GPL symbols
>>> indirectly by whatever is available via EXPORT_SYMBOL(). I don't
>>> really see how that will be better.
>>
>> You are saying that you _know_ companies will violate our license, so
>> you should just "give up"? And how do you know people aren't working on
>> preventing those "indirect" usages as well? :)
>>
>> Sorry, I'm not going to give up here, again, it has proven to work in
>> the past in changing the ways of _very_ large companies, why stop now?
>
> I've found large companies shipping lots of hw putting pressure on
> other large/small companies seems to be only way this has ever
> happened, we'd like to cover that up and say its some great GPL
> enforcement thing.
>
> To be honest, author's choice is how I'd treat this.
>
> Personally I think _GPL is broken by design, and that Linus's initial
> point for them has been so diluted by random lobby groups asking for
> every symbol to be _GPL that they are becoming effectively pointless
> now. I also dislike the fact that the lobby groups don't just bring
> violators to court. I'm also sure someone like the LF could have a
> nice income stream if Linus gave them permission to enforce his
> copyrights.
>
> But anyways, has someone checked that iOS or Windows don't have a
> fence interface? so we know that this is a Linux only interface and
> any works using it are derived? Say the nvidia driver isn't a derived
> work now, will using this interface magically translate it into a
> derived work, so we can go sue them? I don't think so.
I've no ideas about what the APIs are in windows, but windows has had
multi-gpu support for a *long* time, which implies some mechanism like
dmabuf and fence.. this isn't exactly an area where we are
trailblazing here.
BR,
-R
> But its up to Maarten and Rob, and if they say no _GPL then I don't
> think we should be overriding authors intents.
>
> Dave.
--
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: [REPOST PATCH 1/8] fence: dma-buf cross-device synchronization (v17)
On Thu, 2014-06-19 at 15:39 -0700, H. Peter Anvin wrote: > On 06/19/2014 01:01 PM, Greg KH wrote: > > On Thu, Jun 19, 2014 at 09:15:36PM +0200, Daniel Vetter wrote: > >> On Thu, Jun 19, 2014 at 7:00 PM, Greg KH > >> wrote: > >> + BUG_ON(f1->context != f2->context); > > > > Nice, you just crashed the kernel, making it impossible to debug or > > recover :( > > agreed, that should probably be 'if (WARN_ON(...)) return NULL;' > > (but at least I wouldn't expect to hit that under console_lock so you > should at least see the last N lines of the backtrace on the screen > ;-)) > >>> > >>> Lots of devices don't have console screens :) > >> > >> Aside: This is a pet peeve of mine and recently I've switched to > >> rejecting all patch that have a BUG_ON, period. > > > > Please do, I have been for a few years now as well for the same reasons > > you cite. > > > > I'm actually concerned about this trend. Downgrading things to WARN_ON > can allow a security bug in the kernel to continue to exist, for > example, or make the error message disappear. Me too. We use BUG_ON in the I/O subsystem where we're forced to violate a guarantee. When the choice is corrupt something or panic the system, I prefer the latter every time. > I am wondering if the right thing here isn't to have a user (command > line?) settable policy as to how to proceed on an assert violation, > instead of hardcoding it at compile time. I'd say it depends on the consequence of the assertion violation. We have assertions that are largely theoretical, ones that govern process internal state (so killing the process mostly sanitizes the system) and a few that imply data loss or data corruption. James -- 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: [REPOST PATCH 1/8] fence: dma-buf cross-device synchronization (v17)
On 06/19/2014 01:01 PM, Greg KH wrote: > On Thu, Jun 19, 2014 at 09:15:36PM +0200, Daniel Vetter wrote: >> On Thu, Jun 19, 2014 at 7:00 PM, Greg KH wrote: >> + BUG_ON(f1->context != f2->context); > > Nice, you just crashed the kernel, making it impossible to debug or > recover :( agreed, that should probably be 'if (WARN_ON(...)) return NULL;' (but at least I wouldn't expect to hit that under console_lock so you should at least see the last N lines of the backtrace on the screen ;-)) >>> >>> Lots of devices don't have console screens :) >> >> Aside: This is a pet peeve of mine and recently I've switched to >> rejecting all patch that have a BUG_ON, period. > > Please do, I have been for a few years now as well for the same reasons > you cite. > I'm actually concerned about this trend. Downgrading things to WARN_ON can allow a security bug in the kernel to continue to exist, for example, or make the error message disappear. I am wondering if the right thing here isn't to have a user (command line?) settable policy as to how to proceed on an assert violation, instead of hardcoding it at compile time. -hpa -- 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
[no subject]
Is there someone willing to assist me in finding out why my video configuration will not work? I am using a Sony HDR-SR11 as a webcam. Camorama does show video. Other video tools recognize it. Skype does not recognize it. Yahoo messenger shows video once I changed the video preference to Composite. The driver I am using is stk1160 on Ubuntu 12.04 In WebRTC (https://appear.in), stk1160 does show in the Firefox drop down menu for camera and microphone, but after I select it, it says it cannot connect. I'd appreciate a helping hand. Just beware I know little to nothing about how a web video is created and sent and what is needed to do that. Thanks and I appreciate your help. Ray -- 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: [REPOST PATCH 1/8] fence: dma-buf cross-device synchronization (v17)
On 20 June 2014 04:19, Greg KH wrote:
> On Thu, Jun 19, 2014 at 01:45:30PM -0400, Rob Clark wrote:
>> On Thu, Jun 19, 2014 at 1:00 PM, Greg KH wrote:
>> > On Thu, Jun 19, 2014 at 10:00:18AM -0400, Rob Clark wrote:
>> >> On Wed, Jun 18, 2014 at 9:13 PM, Greg KH
>> >> wrote:
>> >> > On Wed, Jun 18, 2014 at 12:36:54PM +0200, Maarten Lankhorst wrote:
>> >> >> +#define CREATE_TRACE_POINTS
>> >> >> +#include
>> >> >> +
>> >> >> +EXPORT_TRACEPOINT_SYMBOL(fence_annotate_wait_on);
>> >> >> +EXPORT_TRACEPOINT_SYMBOL(fence_emit);
>> >> >
>> >> > Are you really willing to live with these as tracepoints for forever?
>> >> > What is the use of them in debugging? Was it just for debugging the
>> >> > fence code, or for something else?
>> >> >
>> >> >> +/**
>> >> >> + * fence_context_alloc - allocate an array of fence contexts
>> >> >> + * @num: [in]amount of contexts to allocate
>> >> >> + *
>> >> >> + * This function will return the first index of the number of fences
>> >> >> allocated.
>> >> >> + * The fence context is used for setting fence->context to a unique
>> >> >> number.
>> >> >> + */
>> >> >> +unsigned fence_context_alloc(unsigned num)
>> >> >> +{
>> >> >> + BUG_ON(!num);
>> >> >> + return atomic_add_return(num, &fence_context_counter) - num;
>> >> >> +}
>> >> >> +EXPORT_SYMBOL(fence_context_alloc);
>> >> >
>> >> > EXPORT_SYMBOL_GPL()? Same goes for all of the exports in here.
>> >> > Traditionally all of the driver core exports have been with this
>> >> > marking, any objection to making that change here as well?
>> >>
>> >> tbh, I prefer EXPORT_SYMBOL().. well, I'd prefer even more if there
>> >> wasn't even a need for EXPORT_SYMBOL_GPL(), but sadly it is a fact of
>> >> life. We already went through this debate once with dma-buf. We
>> >> aren't going to change $evil_vendor's mind about non-gpl modules. The
>> >> only result will be a more flugly convoluted solution (ie. use syncpt
>> >> EXPORT_SYMBOL() on top of fence EXPORT_SYMBOL_GPL()) just as a
>> >> workaround, with the result that no-one benefits.
>> >
>> > It has been proven that using _GPL() exports have caused companies to
>> > release their code "properly" over the years, so as these really are
>> > Linux-only apis, please change them to be marked this way, it helps
>> > everyone out in the end.
>>
>> Well, maybe that is the true in some cases. But it certainly didn't
>> work out that way for dma-buf. And I think the end result is worse.
>>
>> I don't really like coming down on the side of EXPORT_SYMBOL() instead
>> of EXPORT_SYMBOL_GPL(), but if we do use EXPORT_SYMBOL_GPL() then the
>> result will only be creative workarounds using the _GPL symbols
>> indirectly by whatever is available via EXPORT_SYMBOL(). I don't
>> really see how that will be better.
>
> You are saying that you _know_ companies will violate our license, so
> you should just "give up"? And how do you know people aren't working on
> preventing those "indirect" usages as well? :)
>
> Sorry, I'm not going to give up here, again, it has proven to work in
> the past in changing the ways of _very_ large companies, why stop now?
I've found large companies shipping lots of hw putting pressure on
other large/small companies seems to be only way this has ever
happened, we'd like to cover that up and say its some great GPL
enforcement thing.
To be honest, author's choice is how I'd treat this.
Personally I think _GPL is broken by design, and that Linus's initial
point for them has been so diluted by random lobby groups asking for
every symbol to be _GPL that they are becoming effectively pointless
now. I also dislike the fact that the lobby groups don't just bring
violators to court. I'm also sure someone like the LF could have a
nice income stream if Linus gave them permission to enforce his
copyrights.
But anyways, has someone checked that iOS or Windows don't have a
fence interface? so we know that this is a Linux only interface and
any works using it are derived? Say the nvidia driver isn't a derived
work now, will using this interface magically translate it into a
derived work, so we can go sue them? I don't think so.
But its up to Maarten and Rob, and if they say no _GPL then I don't
think we should be overriding authors intents.
Dave.
--
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: [REPOST PATCH 1/8] fence: dma-buf cross-device synchronization (v17)
On Thu, Jun 19, 2014 at 09:15:36PM +0200, Daniel Vetter wrote: > On Thu, Jun 19, 2014 at 7:00 PM, Greg KH wrote: > >> >> + BUG_ON(f1->context != f2->context); > >> > > >> > Nice, you just crashed the kernel, making it impossible to debug or > >> > recover :( > >> > >> agreed, that should probably be 'if (WARN_ON(...)) return NULL;' > >> > >> (but at least I wouldn't expect to hit that under console_lock so you > >> should at least see the last N lines of the backtrace on the screen > >> ;-)) > > > > Lots of devices don't have console screens :) > > Aside: This is a pet peeve of mine and recently I've switched to > rejecting all patch that have a BUG_ON, period. Please do, I have been for a few years now as well for the same reasons you cite. greg k-h -- 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: [REPOST PATCH 1/8] fence: dma-buf cross-device synchronization (v17)
On Thu, Jun 19, 2014 at 8:19 PM, Greg KH wrote: >> >> > EXPORT_SYMBOL_GPL()? Same goes for all of the exports in here. >> >> > Traditionally all of the driver core exports have been with this >> >> > marking, any objection to making that change here as well? >> >> >> >> tbh, I prefer EXPORT_SYMBOL().. well, I'd prefer even more if there >> >> wasn't even a need for EXPORT_SYMBOL_GPL(), but sadly it is a fact of >> >> life. We already went through this debate once with dma-buf. We >> >> aren't going to change $evil_vendor's mind about non-gpl modules. The >> >> only result will be a more flugly convoluted solution (ie. use syncpt >> >> EXPORT_SYMBOL() on top of fence EXPORT_SYMBOL_GPL()) just as a >> >> workaround, with the result that no-one benefits. >> > >> > It has been proven that using _GPL() exports have caused companies to >> > release their code "properly" over the years, so as these really are >> > Linux-only apis, please change them to be marked this way, it helps >> > everyone out in the end. >> >> Well, maybe that is the true in some cases. But it certainly didn't >> work out that way for dma-buf. And I think the end result is worse. >> >> I don't really like coming down on the side of EXPORT_SYMBOL() instead >> of EXPORT_SYMBOL_GPL(), but if we do use EXPORT_SYMBOL_GPL() then the >> result will only be creative workarounds using the _GPL symbols >> indirectly by whatever is available via EXPORT_SYMBOL(). I don't >> really see how that will be better. > > You are saying that you _know_ companies will violate our license, so > you should just "give up"? And how do you know people aren't working on > preventing those "indirect" usages as well? :) > > Sorry, I'm not going to give up here, again, it has proven to work in > the past in changing the ways of _very_ large companies, why stop now? Dave should chime in here since currently dma-buf is _GPL and the drm_prime.c wrapper for it is not (and he merged that one, contributed from said $vendor). And since we're gfx people everything we do is MIT licensed (that's where X is from after all), so _GPL for for drm stuff really doesn't make a lot of sense for us. ianal and all that applies. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch -- 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: mt9m111: add device-tree suppport
Guennadi Liakhovetski writes:
> Hi Robert,
>> +static int of_get_mt9m111_platform_data(struct device *dev,
>> +struct soc_camera_subdev_desc *desc)
>> +{
>> +return 0;
>> +}
>
> Why do you need this function? I would just drop it.
Yeah, drop it sounds good. I wrote it at the beginning to have a clear
structure, and then didn't use it.
>> +static const struct of_device_id mt9m111_of_match[] = {
>> +{ .compatible = "micron,mt9m111", },
>
> Not a flaw in this patch, but someone might want to add "micron" to
> Documentation/devicetree/bindings/vendor-prefixes.txt
OK, I'll see what I can submit, it should be doable :)
Cheers.
--
Robert
--
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: [REPOST PATCH 1/8] fence: dma-buf cross-device synchronization (v17)
On Thu, Jun 19, 2014 at 7:00 PM, Greg KH wrote: >> >> + BUG_ON(f1->context != f2->context); >> > >> > Nice, you just crashed the kernel, making it impossible to debug or >> > recover :( >> >> agreed, that should probably be 'if (WARN_ON(...)) return NULL;' >> >> (but at least I wouldn't expect to hit that under console_lock so you >> should at least see the last N lines of the backtrace on the screen >> ;-)) > > Lots of devices don't have console screens :) Aside: This is a pet peeve of mine and recently I've switched to rejecting all patch that have a BUG_ON, period. Except when you can prove that the kernel will die in the next few lines and there's nothing you can do about it a WARN_ON is always better - I've wasted _way_ too much time debugging hard hangs because such a "benign" BUG_ON ended up eating my irq handler or a spinlock required by such. Or some other nonsense that makes debugging a royal pita, especially if your remote debugger consists of a frustrated users staring at a hung machine. Cheers, Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch -- 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: mt9m111: add device-tree documentation
Guennadi Liakhovetski writes: > Hi Robert, > > On Sun, 15 Jun 2014, Robert Jarzmik wrote: > >> Add documentation for the Micron mt9m111 image sensor. > > A nitpick: this isn't documentation for the sensor:) This is driver DT > bindings' documentation. OK, for V2 (as its twin in pxa_camera). Cheers. -- Robert -- 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: soc_camera: pxa_camera device-tree support
Guennadi Liakhovetski writes:
> On Sun, 15 Jun 2014, Robert Jarzmik wrote:
>> +static const struct of_device_id pxacamera_dt_ids[] = {
>> +{ .compatible = "mrvl,pxa_camera", },
>
> as Documentation/devicetree/bindings/vendor-prefixes.txt defines, it
> should be "marvell."
OK, I'll ask for confirmation to Haojian and Grant, as marvell and mrvl are both
used, and I have many pending patches. So I'd like to be sure, and then amend
all my patches at once.
>> +dev_info(dev, "RJK: %s()\n", __func__);
>
> I have nothing against attributing work to respective authors, but I don't
> think this makes a lot of sense in the long run in the above form :) Once
> you've verified that your binding is working and this function is working,
> either remove this or make it more informative - maybe at the end of this
> function, also listing a couple of important parameters, that you obtained
> from DT.
Ah, debug leftover. RJK is my "special mark" for "remove me before
submitting". See how well it worked :)
>
>> +err = of_property_read_u32(np, "mclk_10khz",
>> + (u32 *)&pdata->mclk_10khz);
>
> I think we'll be frowned upon for this :) PXA270 doesn't support CCF, does
> it? Even if it doesn't we probably should use the standard
> "clock-frequency" property in any case. Actually, I missed to mention on
> this in my comments to your bindings documentation.
You're right. This should be the normal Hz clock stuff. For V2.
>> pcdev->pdata = pdev->dev.platform_data;
>> +if (&pdev->dev.of_node && !pcdev->pdata)
>> +err = pxa_camera_pdata_from_dt(&pdev->dev, &pdata_dt);
>> +if (err < 0)
>> +return err;
>> +else
>> +pcdev->pdata = &pdata_dt;
>
> This will Oops, if someone decides to dereference pcdev->pdata outside of
> this function. pdata_dt is on stack and you store a pointer to it in your
> device data... But since ->pdata doesn't seem to be used anywhere else in
> this driver, maybe remove that struct member completely?
Yep, obliteration, good for me.
There is indeed no purpose in keeping this pdata in pcdev, its only purpose was
to get mclk and platform_flags. For V2 also.
>> +
>> pcdev->platform_flags = pcdev->pdata->flags;
>> if (!(pcdev->platform_flags & (PXA_CAMERA_DATAWIDTH_8 |
>> PXA_CAMERA_DATAWIDTH_9 | PXA_CAMERA_DATAWIDTH_10))) {
>> @@ -1799,10 +1872,17 @@ static const struct dev_pm_ops pxa_camera_pm = {
>> .resume = pxa_camera_resume,
>> };
>>
>> +static const struct of_device_id pxa_camera_of_match[] = {
>> +{ .compatible = "mrvl,pxa_camera", },
>
> Another thing I failed to comment upon: I think DT should contain only
> hardware descriptions, nothing driver specific, and "pxa_camera" is more a
> name of the driver, than the hardware? Maybe something like
> "marvell,pxa27x-cif" would be suitable?
Yeah, sure, whatever you like.
Thanks for the review.
Cheers.
--
Robert
--
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: [REPOST PATCH 1/8] fence: dma-buf cross-device synchronization (v17)
On Thu, Jun 19, 2014 at 2:19 PM, Greg KH wrote:
> On Thu, Jun 19, 2014 at 01:45:30PM -0400, Rob Clark wrote:
>> On Thu, Jun 19, 2014 at 1:00 PM, Greg KH wrote:
>> > On Thu, Jun 19, 2014 at 10:00:18AM -0400, Rob Clark wrote:
>> >> On Wed, Jun 18, 2014 at 9:13 PM, Greg KH
>> >> wrote:
>> >> > On Wed, Jun 18, 2014 at 12:36:54PM +0200, Maarten Lankhorst wrote:
>> >> >> +#define CREATE_TRACE_POINTS
>> >> >> +#include
>> >> >> +
>> >> >> +EXPORT_TRACEPOINT_SYMBOL(fence_annotate_wait_on);
>> >> >> +EXPORT_TRACEPOINT_SYMBOL(fence_emit);
>> >> >
>> >> > Are you really willing to live with these as tracepoints for forever?
>> >> > What is the use of them in debugging? Was it just for debugging the
>> >> > fence code, or for something else?
>> >> >
>> >> >> +/**
>> >> >> + * fence_context_alloc - allocate an array of fence contexts
>> >> >> + * @num: [in]amount of contexts to allocate
>> >> >> + *
>> >> >> + * This function will return the first index of the number of fences
>> >> >> allocated.
>> >> >> + * The fence context is used for setting fence->context to a unique
>> >> >> number.
>> >> >> + */
>> >> >> +unsigned fence_context_alloc(unsigned num)
>> >> >> +{
>> >> >> + BUG_ON(!num);
>> >> >> + return atomic_add_return(num, &fence_context_counter) - num;
>> >> >> +}
>> >> >> +EXPORT_SYMBOL(fence_context_alloc);
>> >> >
>> >> > EXPORT_SYMBOL_GPL()? Same goes for all of the exports in here.
>> >> > Traditionally all of the driver core exports have been with this
>> >> > marking, any objection to making that change here as well?
>> >>
>> >> tbh, I prefer EXPORT_SYMBOL().. well, I'd prefer even more if there
>> >> wasn't even a need for EXPORT_SYMBOL_GPL(), but sadly it is a fact of
>> >> life. We already went through this debate once with dma-buf. We
>> >> aren't going to change $evil_vendor's mind about non-gpl modules. The
>> >> only result will be a more flugly convoluted solution (ie. use syncpt
>> >> EXPORT_SYMBOL() on top of fence EXPORT_SYMBOL_GPL()) just as a
>> >> workaround, with the result that no-one benefits.
>> >
>> > It has been proven that using _GPL() exports have caused companies to
>> > release their code "properly" over the years, so as these really are
>> > Linux-only apis, please change them to be marked this way, it helps
>> > everyone out in the end.
>>
>> Well, maybe that is the true in some cases. But it certainly didn't
>> work out that way for dma-buf. And I think the end result is worse.
>>
>> I don't really like coming down on the side of EXPORT_SYMBOL() instead
>> of EXPORT_SYMBOL_GPL(), but if we do use EXPORT_SYMBOL_GPL() then the
>> result will only be creative workarounds using the _GPL symbols
>> indirectly by whatever is available via EXPORT_SYMBOL(). I don't
>> really see how that will be better.
>
> You are saying that you _know_ companies will violate our license, so
> you should just "give up"? And how do you know people aren't working on
> preventing those "indirect" usages as well? :)
Well, all I know is what happened with dmabuf. This seems like the
exact same scenario (same vendor, same driver, same use-case).
Not really sure how we could completely prevent indirect usage, given
that drm core and many of the drivers are dual MIT/GPL. (But ofc,
IANAL.)
> Sorry, I'm not going to give up here, again, it has proven to work in
> the past in changing the ways of _very_ large companies, why stop now?
In the general case, I would agree. But in this specific case, I am
not very optimistic.
That said, it isn't really my loss if it is _GPL().. I don't have to
use or support that particular driver. But given that we have some
history from the same debate with dma-buf, I think it is pretty easy
to infer the result from making fence EXPORT_SYMBOL_GPL().
BR,
-R
> thanks,
>
> greg k-h
--
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: [REPOST PATCH 1/8] fence: dma-buf cross-device synchronization (v17)
On Thu, 2014-06-19 at 11:19 -0700, Greg KH wrote:
> On Thu, Jun 19, 2014 at 01:45:30PM -0400, Rob Clark wrote:
> > On Thu, Jun 19, 2014 at 1:00 PM, Greg KH wrote:
> > > On Thu, Jun 19, 2014 at 10:00:18AM -0400, Rob Clark wrote:
> > >> On Wed, Jun 18, 2014 at 9:13 PM, Greg KH
> > >> wrote:
> > >> > On Wed, Jun 18, 2014 at 12:36:54PM +0200, Maarten Lankhorst wrote:
> > >> >> +#define CREATE_TRACE_POINTS
> > >> >> +#include
> > >> >> +
> > >> >> +EXPORT_TRACEPOINT_SYMBOL(fence_annotate_wait_on);
> > >> >> +EXPORT_TRACEPOINT_SYMBOL(fence_emit);
> > >> >
> > >> > Are you really willing to live with these as tracepoints for forever?
> > >> > What is the use of them in debugging? Was it just for debugging the
> > >> > fence code, or for something else?
> > >> >
> > >> >> +/**
> > >> >> + * fence_context_alloc - allocate an array of fence contexts
> > >> >> + * @num: [in]amount of contexts to allocate
> > >> >> + *
> > >> >> + * This function will return the first index of the number of fences
> > >> >> allocated.
> > >> >> + * The fence context is used for setting fence->context to a unique
> > >> >> number.
> > >> >> + */
> > >> >> +unsigned fence_context_alloc(unsigned num)
> > >> >> +{
> > >> >> + BUG_ON(!num);
> > >> >> + return atomic_add_return(num, &fence_context_counter) - num;
> > >> >> +}
> > >> >> +EXPORT_SYMBOL(fence_context_alloc);
> > >> >
> > >> > EXPORT_SYMBOL_GPL()? Same goes for all of the exports in here.
> > >> > Traditionally all of the driver core exports have been with this
> > >> > marking, any objection to making that change here as well?
> > >>
> > >> tbh, I prefer EXPORT_SYMBOL().. well, I'd prefer even more if there
> > >> wasn't even a need for EXPORT_SYMBOL_GPL(), but sadly it is a fact of
> > >> life. We already went through this debate once with dma-buf. We
> > >> aren't going to change $evil_vendor's mind about non-gpl modules. The
> > >> only result will be a more flugly convoluted solution (ie. use syncpt
> > >> EXPORT_SYMBOL() on top of fence EXPORT_SYMBOL_GPL()) just as a
> > >> workaround, with the result that no-one benefits.
> > >
> > > It has been proven that using _GPL() exports have caused companies to
> > > release their code "properly" over the years, so as these really are
> > > Linux-only apis, please change them to be marked this way, it helps
> > > everyone out in the end.
> >
> > Well, maybe that is the true in some cases. But it certainly didn't
> > work out that way for dma-buf. And I think the end result is worse.
> >
> > I don't really like coming down on the side of EXPORT_SYMBOL() instead
> > of EXPORT_SYMBOL_GPL(), but if we do use EXPORT_SYMBOL_GPL() then the
> > result will only be creative workarounds using the _GPL symbols
> > indirectly by whatever is available via EXPORT_SYMBOL(). I don't
> > really see how that will be better.
>
> You are saying that you _know_ companies will violate our license, so
> you should just "give up"? And how do you know people aren't working on
> preventing those "indirect" usages as well? :)
>
> Sorry, I'm not going to give up here, again, it has proven to work in
> the past in changing the ways of _very_ large companies, why stop now?
When you try to train a dog, you have to be consistent about it. We're
fantastically inconsistent in symbol exports.
For instance, the mutex primitives are all EXPORT_SYMBOL(), so we're
telling proprietary modules they can use them. However, when the kernel
is built with CONFIG_DEBUG_MUTEX, they all become
EXPORT_SYMBOL_GPL() ... what type of crazy message is that supposed to
send? It's OK to use mutexes but it's potentially a GPL violation to
debug them?
We either need to decide that we have a defined and consistent part of
our API that's GPL only or make the bold statement that we don't have
any part of our API that's usable by non-GPL modules. Right at the
minute we do neither and it confuses people no end about what is and
isn't allowed.
James
--
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: [REPOST PATCH 1/8] fence: dma-buf cross-device synchronization (v17)
On Thu, Jun 19, 2014 at 01:45:30PM -0400, Rob Clark wrote:
> On Thu, Jun 19, 2014 at 1:00 PM, Greg KH wrote:
> > On Thu, Jun 19, 2014 at 10:00:18AM -0400, Rob Clark wrote:
> >> On Wed, Jun 18, 2014 at 9:13 PM, Greg KH
> >> wrote:
> >> > On Wed, Jun 18, 2014 at 12:36:54PM +0200, Maarten Lankhorst wrote:
> >> >> +#define CREATE_TRACE_POINTS
> >> >> +#include
> >> >> +
> >> >> +EXPORT_TRACEPOINT_SYMBOL(fence_annotate_wait_on);
> >> >> +EXPORT_TRACEPOINT_SYMBOL(fence_emit);
> >> >
> >> > Are you really willing to live with these as tracepoints for forever?
> >> > What is the use of them in debugging? Was it just for debugging the
> >> > fence code, or for something else?
> >> >
> >> >> +/**
> >> >> + * fence_context_alloc - allocate an array of fence contexts
> >> >> + * @num: [in]amount of contexts to allocate
> >> >> + *
> >> >> + * This function will return the first index of the number of fences
> >> >> allocated.
> >> >> + * The fence context is used for setting fence->context to a unique
> >> >> number.
> >> >> + */
> >> >> +unsigned fence_context_alloc(unsigned num)
> >> >> +{
> >> >> + BUG_ON(!num);
> >> >> + return atomic_add_return(num, &fence_context_counter) - num;
> >> >> +}
> >> >> +EXPORT_SYMBOL(fence_context_alloc);
> >> >
> >> > EXPORT_SYMBOL_GPL()? Same goes for all of the exports in here.
> >> > Traditionally all of the driver core exports have been with this
> >> > marking, any objection to making that change here as well?
> >>
> >> tbh, I prefer EXPORT_SYMBOL().. well, I'd prefer even more if there
> >> wasn't even a need for EXPORT_SYMBOL_GPL(), but sadly it is a fact of
> >> life. We already went through this debate once with dma-buf. We
> >> aren't going to change $evil_vendor's mind about non-gpl modules. The
> >> only result will be a more flugly convoluted solution (ie. use syncpt
> >> EXPORT_SYMBOL() on top of fence EXPORT_SYMBOL_GPL()) just as a
> >> workaround, with the result that no-one benefits.
> >
> > It has been proven that using _GPL() exports have caused companies to
> > release their code "properly" over the years, so as these really are
> > Linux-only apis, please change them to be marked this way, it helps
> > everyone out in the end.
>
> Well, maybe that is the true in some cases. But it certainly didn't
> work out that way for dma-buf. And I think the end result is worse.
>
> I don't really like coming down on the side of EXPORT_SYMBOL() instead
> of EXPORT_SYMBOL_GPL(), but if we do use EXPORT_SYMBOL_GPL() then the
> result will only be creative workarounds using the _GPL symbols
> indirectly by whatever is available via EXPORT_SYMBOL(). I don't
> really see how that will be better.
You are saying that you _know_ companies will violate our license, so
you should just "give up"? And how do you know people aren't working on
preventing those "indirect" usages as well? :)
Sorry, I'm not going to give up here, again, it has proven to work in
the past in changing the ways of _very_ large companies, why stop now?
thanks,
greg k-h
--
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: [REPOST PATCH 1/8] fence: dma-buf cross-device synchronization (v17)
On Thu, Jun 19, 2014 at 1:00 PM, Greg KH wrote:
> On Thu, Jun 19, 2014 at 10:00:18AM -0400, Rob Clark wrote:
>> On Wed, Jun 18, 2014 at 9:13 PM, Greg KH wrote:
>> > On Wed, Jun 18, 2014 at 12:36:54PM +0200, Maarten Lankhorst wrote:
>> >> +#define CREATE_TRACE_POINTS
>> >> +#include
>> >> +
>> >> +EXPORT_TRACEPOINT_SYMBOL(fence_annotate_wait_on);
>> >> +EXPORT_TRACEPOINT_SYMBOL(fence_emit);
>> >
>> > Are you really willing to live with these as tracepoints for forever?
>> > What is the use of them in debugging? Was it just for debugging the
>> > fence code, or for something else?
>> >
>> >> +/**
>> >> + * fence_context_alloc - allocate an array of fence contexts
>> >> + * @num: [in]amount of contexts to allocate
>> >> + *
>> >> + * This function will return the first index of the number of fences
>> >> allocated.
>> >> + * The fence context is used for setting fence->context to a unique
>> >> number.
>> >> + */
>> >> +unsigned fence_context_alloc(unsigned num)
>> >> +{
>> >> + BUG_ON(!num);
>> >> + return atomic_add_return(num, &fence_context_counter) - num;
>> >> +}
>> >> +EXPORT_SYMBOL(fence_context_alloc);
>> >
>> > EXPORT_SYMBOL_GPL()? Same goes for all of the exports in here.
>> > Traditionally all of the driver core exports have been with this
>> > marking, any objection to making that change here as well?
>>
>> tbh, I prefer EXPORT_SYMBOL().. well, I'd prefer even more if there
>> wasn't even a need for EXPORT_SYMBOL_GPL(), but sadly it is a fact of
>> life. We already went through this debate once with dma-buf. We
>> aren't going to change $evil_vendor's mind about non-gpl modules. The
>> only result will be a more flugly convoluted solution (ie. use syncpt
>> EXPORT_SYMBOL() on top of fence EXPORT_SYMBOL_GPL()) just as a
>> workaround, with the result that no-one benefits.
>
> It has been proven that using _GPL() exports have caused companies to
> release their code "properly" over the years, so as these really are
> Linux-only apis, please change them to be marked this way, it helps
> everyone out in the end.
Well, maybe that is the true in some cases. But it certainly didn't
work out that way for dma-buf. And I think the end result is worse.
I don't really like coming down on the side of EXPORT_SYMBOL() instead
of EXPORT_SYMBOL_GPL(), but if we do use EXPORT_SYMBOL_GPL() then the
result will only be creative workarounds using the _GPL symbols
indirectly by whatever is available via EXPORT_SYMBOL(). I don't
really see how that will be better.
>> >> +int __fence_signal(struct fence *fence)
>> >> +{
>> >> + struct fence_cb *cur, *tmp;
>> >> + int ret = 0;
>> >> +
>> >> + if (WARN_ON(!fence))
>> >> + return -EINVAL;
>> >> +
>> >> + if (!ktime_to_ns(fence->timestamp)) {
>> >> + fence->timestamp = ktime_get();
>> >> + smp_mb__before_atomic();
>> >> + }
>> >> +
>> >> + if (test_and_set_bit(FENCE_FLAG_SIGNALED_BIT, &fence->flags)) {
>> >> + ret = -EINVAL;
>> >> +
>> >> + /*
>> >> + * we might have raced with the unlocked fence_signal,
>> >> + * still run through all callbacks
>> >> + */
>> >> + } else
>> >> + trace_fence_signaled(fence);
>> >> +
>> >> + list_for_each_entry_safe(cur, tmp, &fence->cb_list, node) {
>> >> + list_del_init(&cur->node);
>> >> + cur->func(fence, cur);
>> >> + }
>> >> + return ret;
>> >> +}
>> >> +EXPORT_SYMBOL(__fence_signal);
>> >
>> > Don't export a function with __ in front of it, you are saying that an
>> > internal function is somehow "valid" for everyone else to call? Why?
>> > You aren't even documenting the function, so who knows how to use it?
>>
>> fwiw, the __ versions appear to mainly be concessions for android
>> syncpt. That is the only user outside of fence.c, and it should stay
>> that way.
>
> How are you going to "ensure" this? And where did you document it?
> Please fix this up, it's a horrid way to create a new api.
>
> If the android code needs to be fixed to fit into this model, then fix
> it.
heh, and in fact I was wrong about this.. the __ versions are actually
for when the lock is already held. Maarten needs to rename (ie
_locked suffix) and add some API docs for this.
>> >> +void
>> >> +__fence_init(struct fence *fence, const struct fence_ops *ops,
>> >> + spinlock_t *lock, unsigned context, unsigned seqno)
>> >> +{
>> >> + BUG_ON(!lock);
>> >> + BUG_ON(!ops || !ops->wait || !ops->enable_signaling ||
>> >> +!ops->get_driver_name || !ops->get_timeline_name);
>> >> +
>> >> + kref_init(&fence->refcount);
>> >> + fence->ops = ops;
>> >> + INIT_LIST_HEAD(&fence->cb_list);
>> >> + fence->lock = lock;
>> >> + fence->context = context;
>> >> + fence->seqno = seqno;
>> >> + fence->flags = 0UL;
>> >> +
>> >> + trace_fence_init(fence);
>> >> +}
>> >> +EXPORT_SYMBOL(__fence_init);
>> >
>> > Agai
[REVIEW PATCH 3/4] media: v4l2-dev.h: remove V4L2_FL_USE_FH_PRIO flag.
From: Ramakrishnan Muthukrishnan Since none of the drivers are using it, this flag can be removed. Signed-off-by: Ramakrishnan Muthukrishnan --- include/media/v4l2-dev.h | 2 -- 1 file changed, 2 deletions(-) diff --git a/include/media/v4l2-dev.h b/include/media/v4l2-dev.h index eec6e46..eb76cfd 100644 --- a/include/media/v4l2-dev.h +++ b/include/media/v4l2-dev.h @@ -44,8 +44,6 @@ struct v4l2_ctrl_handler; #define V4L2_FL_REGISTERED (0) /* file->private_data points to struct v4l2_fh */ #define V4L2_FL_USES_V4L2_FH (1) -/* Use the prio field of v4l2_fh for core priority checking */ -#define V4L2_FL_USE_FH_PRIO(2) /* Priority helper functions */ -- 2.0.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
[REVIEW PATCH 4/4] media: Documentation: remove V4L2_FL_USE_FH_PRIO flag.
From: Ramakrishnan Muthukrishnan Signed-off-by: Ramakrishnan Muthukrishnan --- Documentation/video4linux/v4l2-framework.txt | 8 +--- Documentation/video4linux/v4l2-pci-skeleton.c | 5 - Documentation/zh_CN/video4linux/v4l2-framework.txt | 7 +-- 3 files changed, 2 insertions(+), 18 deletions(-) diff --git a/Documentation/video4linux/v4l2-framework.txt b/Documentation/video4linux/v4l2-framework.txt index 667a433..a11dff0 100644 --- a/Documentation/video4linux/v4l2-framework.txt +++ b/Documentation/video4linux/v4l2-framework.txt @@ -675,11 +675,6 @@ You should also set these fields: video_device is initialized you *do* know which parent PCI device to use and so you set dev_device to the correct PCI device. -- flags: optional. Set to V4L2_FL_USE_FH_PRIO if you want to let the framework - handle the VIDIOC_G/S_PRIORITY ioctls. This requires that you use struct - v4l2_fh. Eventually this flag will disappear once all drivers use the core - priority handling. But for now it has to be set explicitly. - If you use v4l2_ioctl_ops, then you should set .unlocked_ioctl to video_ioctl2 in your v4l2_file_operations struct. @@ -909,8 +904,7 @@ struct v4l2_fh struct v4l2_fh provides a way to easily keep file handle specific data that is used by the V4L2 framework. New drivers must use struct v4l2_fh -since it is also used to implement priority handling (VIDIOC_G/S_PRIORITY) -if the video_device flag V4L2_FL_USE_FH_PRIO is also set. +since it is also used to implement priority handling (VIDIOC_G/S_PRIORITY). The users of v4l2_fh (in the V4L2 framework, not the driver) know whether a driver uses v4l2_fh as its file->private_data pointer by diff --git a/Documentation/video4linux/v4l2-pci-skeleton.c b/Documentation/video4linux/v4l2-pci-skeleton.c index 46904fe..006721e 100644 --- a/Documentation/video4linux/v4l2-pci-skeleton.c +++ b/Documentation/video4linux/v4l2-pci-skeleton.c @@ -883,11 +883,6 @@ static int skeleton_probe(struct pci_dev *pdev, const struct pci_device_id *ent) vdev->v4l2_dev = &skel->v4l2_dev; /* Supported SDTV standards, if any */ vdev->tvnorms = SKEL_TVNORMS; - /* If this bit is set, then the v4l2 core will provide the support -* for the VIDIOC_G/S_PRIORITY ioctls. This flag will eventually -* go away once all drivers have been converted to use struct v4l2_fh. -*/ - set_bit(V4L2_FL_USE_FH_PRIO, &vdev->flags); video_set_drvdata(vdev, skel); ret = video_register_device(vdev, VFL_TYPE_GRABBER, -1); diff --git a/Documentation/zh_CN/video4linux/v4l2-framework.txt b/Documentation/zh_CN/video4linux/v4l2-framework.txt index 0da95db..2b828e6 100644 --- a/Documentation/zh_CN/video4linux/v4l2-framework.txt +++ b/Documentation/zh_CN/video4linux/v4l2-framework.txt @@ -580,11 +580,6 @@ release()回调必须被设置，且在最后一个 video_device 用户退出之 v4l2_device 无法与特定的 PCI 设备关联，所有没有设置父设备。但当 video_device 配置后，就知道使用哪个父 PCI 设备了。 -- flags：可选。如果你要让框架处理设置 VIDIOC_G/S_PRIORITY ioctls， - 请设置 V4L2_FL_USE_FH_PRIO。这要求你使用 v4l2_fh 结构体。 - 一旦所有驱动使用了核心的优先级处理，最终这个标志将消失。但现在它 - 必须被显式设置。 - 如果你使用 v4l2_ioctl_ops，则应该在 v4l2_file_operations 结构体中 设置 .unlocked_ioctl 指向 video_ioctl2。 @@ -789,7 +784,7 @@ v4l2_fh 结构体 - v4l2_fh 结构体提供一个保存用于 V4L2 框架的文件句柄特定数据的简单方法。 -如果 video_device 的 flag 设置了 V4L2_FL_USE_FH_PRIO 标志，新驱动 +如果 video_device 标志，新驱动 必须使用 v4l2_fh 结构体，因为它也用于实现优先级处理（VIDIOC_G/S_PRIORITY）。 v4l2_fh 的用户（位于 V4l2 框架中，并非驱动）可通过测试 -- 2.0.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
[REVIEW PATCH 1/4] media: v4l2-core: remove the use of V4L2_FL_USE_FH_PRIO flag.
From: Ramakrishnan Muthukrishnan
Since all the drivers that use `struct v4l2_fh' use the core priority
checking instead of doing it themselves, this flag can be removed.
This patch removes the usage of the flag from v4l2-core.
Signed-off-by: Ramakrishnan Muthukrishnan
---
drivers/media/v4l2-core/v4l2-dev.c | 6 ++
drivers/media/v4l2-core/v4l2-fh.c| 13 +
drivers/media/v4l2-core/v4l2-ioctl.c | 9 +++--
3 files changed, 14 insertions(+), 14 deletions(-)
diff --git a/drivers/media/v4l2-core/v4l2-dev.c
b/drivers/media/v4l2-core/v4l2-dev.c
index 634d863..35698aa 100644
--- a/drivers/media/v4l2-core/v4l2-dev.c
+++ b/drivers/media/v4l2-core/v4l2-dev.c
@@ -563,11 +563,9 @@ static void determine_valid_ioctls(struct video_device
*vdev)
/* vfl_type and vfl_dir independent ioctls */
SET_VALID_IOCTL(ops, VIDIOC_QUERYCAP, vidioc_querycap);
- if (ops->vidioc_g_priority ||
- test_bit(V4L2_FL_USE_FH_PRIO, &vdev->flags))
+ if (ops->vidioc_g_priority)
set_bit(_IOC_NR(VIDIOC_G_PRIORITY), valid_ioctls);
- if (ops->vidioc_s_priority ||
- test_bit(V4L2_FL_USE_FH_PRIO, &vdev->flags))
+ if (ops->vidioc_s_priority)
set_bit(_IOC_NR(VIDIOC_S_PRIORITY), valid_ioctls);
SET_VALID_IOCTL(ops, VIDIOC_STREAMON, vidioc_streamon);
SET_VALID_IOCTL(ops, VIDIOC_STREAMOFF, vidioc_streamoff);
diff --git a/drivers/media/v4l2-core/v4l2-fh.c
b/drivers/media/v4l2-core/v4l2-fh.c
index e57c002..c97067a 100644
--- a/drivers/media/v4l2-core/v4l2-fh.c
+++ b/drivers/media/v4l2-core/v4l2-fh.c
@@ -37,6 +37,13 @@ void v4l2_fh_init(struct v4l2_fh *fh, struct video_device
*vdev)
fh->ctrl_handler = vdev->ctrl_handler;
INIT_LIST_HEAD(&fh->list);
set_bit(V4L2_FL_USES_V4L2_FH, &fh->vdev->flags);
+ /*
+* determine_valid_ioctls() does not know if struct v4l2_fh
+* is used by this driver, but here we do. So enable the
+* prio ioctls here.
+*/
+ set_bit(_IOC_NR(VIDIOC_G_PRIORITY), vdev->valid_ioctls);
+ set_bit(_IOC_NR(VIDIOC_S_PRIORITY), vdev->valid_ioctls);
fh->prio = V4L2_PRIORITY_UNSET;
init_waitqueue_head(&fh->wait);
INIT_LIST_HEAD(&fh->available);
@@ -49,8 +56,7 @@ void v4l2_fh_add(struct v4l2_fh *fh)
{
unsigned long flags;
- if (test_bit(V4L2_FL_USE_FH_PRIO, &fh->vdev->flags))
- v4l2_prio_open(fh->vdev->prio, &fh->prio);
+ v4l2_prio_open(fh->vdev->prio, &fh->prio);
spin_lock_irqsave(&fh->vdev->fh_lock, flags);
list_add(&fh->list, &fh->vdev->fh_list);
spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
@@ -78,8 +84,7 @@ void v4l2_fh_del(struct v4l2_fh *fh)
spin_lock_irqsave(&fh->vdev->fh_lock, flags);
list_del_init(&fh->list);
spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
- if (test_bit(V4L2_FL_USE_FH_PRIO, &fh->vdev->flags))
- v4l2_prio_close(fh->vdev->prio, fh->prio);
+ v4l2_prio_close(fh->vdev->prio, fh->prio);
}
EXPORT_SYMBOL_GPL(v4l2_fh_del);
diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c
b/drivers/media/v4l2-core/v4l2-ioctl.c
index 16bffd8..8d4a25d 100644
--- a/drivers/media/v4l2-core/v4l2-ioctl.c
+++ b/drivers/media/v4l2-core/v4l2-ioctl.c
@@ -2190,7 +2190,6 @@ static long __video_do_ioctl(struct file *file,
const struct v4l2_ioctl_info *info;
void *fh = file->private_data;
struct v4l2_fh *vfh = NULL;
- int use_fh_prio = 0;
int debug = vfd->debug;
long ret = -ENOTTY;
@@ -2200,10 +2199,8 @@ static long __video_do_ioctl(struct file *file,
return ret;
}
- if (test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags)) {
+ if (test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags))
vfh = file->private_data;
- use_fh_prio = test_bit(V4L2_FL_USE_FH_PRIO, &vfd->flags);
- }
if (v4l2_is_known_ioctl(cmd)) {
info = &v4l2_ioctls[_IOC_NR(cmd)];
@@ -2212,7 +2209,7 @@ static long __video_do_ioctl(struct file *file,
!((info->flags & INFO_FL_CTRL) && vfh && vfh->ctrl_handler))
goto done;
- if (use_fh_prio && (info->flags & INFO_FL_PRIO)) {
+ if (vfh && (info->flags & INFO_FL_PRIO)) {
ret = v4l2_prio_check(vfd->prio, vfh->prio);
if (ret)
goto done;
@@ -2237,7 +2234,7 @@ static long __video_do_ioctl(struct file *file,
ret = -ENOTTY;
} else {
ret = ops->vidioc_default(file, fh,
- use_fh_prio ? v4l2_prio_check(vfd->prio, vfh->prio) >=
0 : 0,
+ vfh ? v4l2_prio_check(vfd->prio, vfh->prio) >= 0 : 0,
cmd, arg);
}
--
2.0.0
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body
[REVIEW PATCH 2/4] media: remove the setting of the flag V4L2_FL_USE_FH_PRIO.
From: Ramakrishnan Muthukrishnan Since all the drivers that use `struct v4l2_fh' use the core priority checking, the setting of the flag in the drivers can be removed. Signed-off-by: Ramakrishnan Muthukrishnan --- drivers/media/common/saa7146/saa7146_fops.c| 1 - drivers/media/parport/bw-qcam.c| 1 - drivers/media/parport/c-qcam.c | 1 - drivers/media/parport/pms.c| 1 - drivers/media/parport/w9966.c | 1 - drivers/media/pci/bt8xx/bttv-driver.c | 1 - drivers/media/pci/cx18/cx18-streams.c | 1 - drivers/media/pci/cx25821/cx25821-video.c | 1 - drivers/media/pci/cx88/cx88-core.c | 1 - drivers/media/pci/ivtv/ivtv-streams.c | 1 - drivers/media/pci/meye/meye.c | 1 - drivers/media/pci/saa7134/saa7134-core.c | 1 - drivers/media/pci/saa7134/saa7134-empress.c| 1 - drivers/media/pci/sta2x11/sta2x11_vip.c| 1 - drivers/media/platform/arv.c | 1 - drivers/media/platform/blackfin/bfin_capture.c | 1 - drivers/media/platform/davinci/vpbe_display.c | 1 - drivers/media/platform/davinci/vpfe_capture.c | 1 - drivers/media/platform/davinci/vpif_capture.c | 1 - drivers/media/platform/davinci/vpif_display.c | 1 - drivers/media/platform/s3c-camif/camif-capture.c | 1 - drivers/media/platform/s5p-tv/mixer_video.c| 2 -- drivers/media/platform/vivi.c | 1 - drivers/media/radio/dsbr100.c | 1 - drivers/media/radio/radio-cadet.c | 1 - drivers/media/radio/radio-isa.c| 1 - drivers/media/radio/radio-keene.c | 1 - drivers/media/radio/radio-ma901.c | 1 - drivers/media/radio/radio-miropcm20.c | 1 - drivers/media/radio/radio-mr800.c | 1 - drivers/media/radio/radio-raremono.c | 1 - drivers/media/radio/radio-sf16fmi.c| 1 - drivers/media/radio/radio-si476x.c | 1 - drivers/media/radio/radio-tea5764.c| 1 - drivers/media/radio/radio-tea5777.c| 1 - drivers/media/radio/radio-timb.c | 1 - drivers/media/radio/si470x/radio-si470x-usb.c | 1 - drivers/media/radio/si4713/radio-platform-si4713.c | 1 - drivers/media/radio/si4713/radio-usb-si4713.c | 1 - drivers/media/radio/tea575x.c | 1 - drivers/media/usb/au0828/au0828-video.c| 2 -- drivers/media/usb/cpia2/cpia2_v4l.c| 1 - drivers/media/usb/cx231xx/cx231xx-417.c| 1 - drivers/media/usb/cx231xx/cx231xx-video.c | 1 - drivers/media/usb/em28xx/em28xx-video.c| 1 - drivers/media/usb/gspca/gspca.c| 1 - drivers/media/usb/hdpvr/hdpvr-video.c | 1 - drivers/media/usb/pwc/pwc-if.c | 1 - drivers/media/usb/s2255/s2255drv.c | 1 - drivers/media/usb/stk1160/stk1160-v4l.c| 1 - drivers/media/usb/stkwebcam/stk-webcam.c | 1 - drivers/media/usb/tlg2300/pd-radio.c | 1 - drivers/media/usb/tm6000/tm6000-video.c| 1 - drivers/media/usb/usbtv/usbtv-video.c | 1 - drivers/media/usb/uvc/uvc_driver.c | 1 - drivers/media/usb/zr364xx/zr364xx.c| 1 - drivers/staging/media/davinci_vpfe/vpfe_video.c| 1 - drivers/staging/media/go7007/go7007-v4l2.c | 1 - drivers/staging/media/msi3101/sdr-msi3101.c| 1 - drivers/staging/media/rtl2832u_sdr/rtl2832_sdr.c | 1 - drivers/staging/media/solo6x10/solo6x10-v4l2-enc.c | 1 - drivers/staging/media/solo6x10/solo6x10-v4l2.c | 1 - 62 files changed, 64 deletions(-) diff --git a/drivers/media/common/saa7146/saa7146_fops.c b/drivers/media/common/saa7146/saa7146_fops.c index eda01bc..f2cc521 100644 --- a/drivers/media/common/saa7146/saa7146_fops.c +++ b/drivers/media/common/saa7146/saa7146_fops.c @@ -613,7 +613,6 @@ int saa7146_register_device(struct video_device **vid, struct saa7146_dev* dev, vfd->lock = &dev->v4l2_lock; vfd->v4l2_dev = &dev->v4l2_dev; vfd->tvnorms = 0; - set_bit(V4L2_FL_USE_FH_PRIO, &vfd->flags); for (i = 0; i < dev->ext_vv_data->num_stds; i++) vfd->tvnorms |= dev->ext_vv_data->stds[i].id; strlcpy(vfd->name, name, sizeof(vfd->name)); diff --git a/drivers/media/parport/bw-qcam.c b/drivers/media/parport/bw-qcam.c index 416507a..3c5dac4 100644 --- a/drivers/media/parport/bw-qcam.c +++ b/drivers/media/parport/bw-qcam.c @@ -990,7 +990,6 @@ static struct qcam *qcam_init(struct parport *port) qcam->vdev.fops = &qcam_fops; qcam->vdev.lock = &qcam->lock; qcam->vdev.ioctl_ops = &qcam_ioctl_ops; - set_bit(V4L2_FL_USE_FH_PRIO, &qcam->vdev.flags); qcam->vdev.release = video_device_relea
[REVIEW PATCH] media: remove V4L2_FL_USE_FH_PRIO flag and its usage.
Since all the drivers that use `struct v4l2_fh' use the core priority checking, instead of doing it themselves, the flag V4L2_FL_USE_FH_PRIO can be removed. This patch series removes the use of this flag in the v4l2-core and also removes the setting of this flag from the drivers and finally removes the flag itself. -- 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: [REPOST PATCH 1/8] fence: dma-buf cross-device synchronization (v17)
On Thu, Jun 19, 2014 at 10:00:18AM -0400, Rob Clark wrote:
> On Wed, Jun 18, 2014 at 9:13 PM, Greg KH wrote:
> > On Wed, Jun 18, 2014 at 12:36:54PM +0200, Maarten Lankhorst wrote:
> >> +#define CREATE_TRACE_POINTS
> >> +#include
> >> +
> >> +EXPORT_TRACEPOINT_SYMBOL(fence_annotate_wait_on);
> >> +EXPORT_TRACEPOINT_SYMBOL(fence_emit);
> >
> > Are you really willing to live with these as tracepoints for forever?
> > What is the use of them in debugging? Was it just for debugging the
> > fence code, or for something else?
> >
> >> +/**
> >> + * fence_context_alloc - allocate an array of fence contexts
> >> + * @num: [in]amount of contexts to allocate
> >> + *
> >> + * This function will return the first index of the number of fences
> >> allocated.
> >> + * The fence context is used for setting fence->context to a unique
> >> number.
> >> + */
> >> +unsigned fence_context_alloc(unsigned num)
> >> +{
> >> + BUG_ON(!num);
> >> + return atomic_add_return(num, &fence_context_counter) - num;
> >> +}
> >> +EXPORT_SYMBOL(fence_context_alloc);
> >
> > EXPORT_SYMBOL_GPL()? Same goes for all of the exports in here.
> > Traditionally all of the driver core exports have been with this
> > marking, any objection to making that change here as well?
>
> tbh, I prefer EXPORT_SYMBOL().. well, I'd prefer even more if there
> wasn't even a need for EXPORT_SYMBOL_GPL(), but sadly it is a fact of
> life. We already went through this debate once with dma-buf. We
> aren't going to change $evil_vendor's mind about non-gpl modules. The
> only result will be a more flugly convoluted solution (ie. use syncpt
> EXPORT_SYMBOL() on top of fence EXPORT_SYMBOL_GPL()) just as a
> workaround, with the result that no-one benefits.
It has been proven that using _GPL() exports have caused companies to
release their code "properly" over the years, so as these really are
Linux-only apis, please change them to be marked this way, it helps
everyone out in the end.
> >> +int __fence_signal(struct fence *fence)
> >> +{
> >> + struct fence_cb *cur, *tmp;
> >> + int ret = 0;
> >> +
> >> + if (WARN_ON(!fence))
> >> + return -EINVAL;
> >> +
> >> + if (!ktime_to_ns(fence->timestamp)) {
> >> + fence->timestamp = ktime_get();
> >> + smp_mb__before_atomic();
> >> + }
> >> +
> >> + if (test_and_set_bit(FENCE_FLAG_SIGNALED_BIT, &fence->flags)) {
> >> + ret = -EINVAL;
> >> +
> >> + /*
> >> + * we might have raced with the unlocked fence_signal,
> >> + * still run through all callbacks
> >> + */
> >> + } else
> >> + trace_fence_signaled(fence);
> >> +
> >> + list_for_each_entry_safe(cur, tmp, &fence->cb_list, node) {
> >> + list_del_init(&cur->node);
> >> + cur->func(fence, cur);
> >> + }
> >> + return ret;
> >> +}
> >> +EXPORT_SYMBOL(__fence_signal);
> >
> > Don't export a function with __ in front of it, you are saying that an
> > internal function is somehow "valid" for everyone else to call? Why?
> > You aren't even documenting the function, so who knows how to use it?
>
> fwiw, the __ versions appear to mainly be concessions for android
> syncpt. That is the only user outside of fence.c, and it should stay
> that way.
How are you going to "ensure" this? And where did you document it?
Please fix this up, it's a horrid way to create a new api.
If the android code needs to be fixed to fit into this model, then fix
it.
> >> +void
> >> +__fence_init(struct fence *fence, const struct fence_ops *ops,
> >> + spinlock_t *lock, unsigned context, unsigned seqno)
> >> +{
> >> + BUG_ON(!lock);
> >> + BUG_ON(!ops || !ops->wait || !ops->enable_signaling ||
> >> +!ops->get_driver_name || !ops->get_timeline_name);
> >> +
> >> + kref_init(&fence->refcount);
> >> + fence->ops = ops;
> >> + INIT_LIST_HEAD(&fence->cb_list);
> >> + fence->lock = lock;
> >> + fence->context = context;
> >> + fence->seqno = seqno;
> >> + fence->flags = 0UL;
> >> +
> >> + trace_fence_init(fence);
> >> +}
> >> +EXPORT_SYMBOL(__fence_init);
> >
> > Again with the __ exported function...
> >
> > I don't even see a fence_init() function anywhere, why the __ ?
> >
>
> think of it as a 'protected' constructor.. only the derived fence
> subclass should call.
Where do you say this? Again, not a good reason, fix up the api please.
> >> + kref_get(&fence->refcount);
> >> +}
> >
> > Why is this inline?
>
> performance can be critical.. especially if the driver is using this
> fence mechanism for internal buffers as well as shared buffers (which
> is what I'd like to do to avoid having to deal with two different
> fencing mechanisms for shared vs non-shared buffers), since you could
> easily have 100's or perhaps 1000's of buffers involved in a submit.
"can be". Did you actually measure it? Please do so.
> The fence stuff does try
Re: [REPOST PATCH 4/8] android: convert sync to fence api, v5
On Thu, Jun 19, 2014 at 08:35:29AM -0700, Colin Cross wrote: > On Thu, Jun 19, 2014 at 5:28 AM, Daniel Vetter wrote: > > On Thu, Jun 19, 2014 at 1:48 PM, Thierry Reding > > wrote: > >>> > With these changes, can we pull the android sync logic out of > >>> > drivers/staging/ now? > >>> > >>> Afaik the google guys never really looked at this and acked it. So I'm not > >>> sure whether they'll follow along. The other issue I have as the > >>> maintainer of gfx driver is that I don't want to implement support for two > >>> different sync object primitives (once for dma-buf and once for android > >>> syncpts), and my impression thus far has been that even with this we're > >>> not there. > >>> > >>> I'm trying to get our own android guys to upstream their i915 syncpts > >>> support, but thus far I haven't managed to convince them to throw people's > >>> time at this. > >> > >> This has been discussed a fair bit internally recently and some of our > >> GPU experts have raised concerns that this may result in seriously > >> degraded performance in our proprietary graphics stack. Now I don't care > >> very much for the proprietary graphics stack, but by extension I would > >> assume that the same restrictions are relevant for any open-source > >> driver as well. > >> > >> I'm still trying to fully understand all the implications and at the > >> same time get some of the people who raised concerns to join in this > >> discussion. As I understand it the concern is mostly about explicit vs. > >> implicit synchronization and having this mechanism in the kernel will > >> implicitly synchronize all accesses to these buffers even in cases where > >> it's not needed (read vs. write locks, etc.). In one particular instance > >> it was even mentioned that this kind of implicit synchronization can > >> lead to deadlocks in some use-cases (this was mentioned for Android > >> compositing, but I suspect that the same may happen for Wayland or X > >> compositors). > > > > Well the implicit fences here actually can't deadlock. That's the > > entire point behind using ww mutexes. I've also heard tons of > > complaints about implicit enforced syncing (especially from opencl > > people), but in the end drivers and always expose unsynchronized > > access for specific cases. We do that in i915 for upload buffers and > > other fun stuff. This is about shared stuff across different drivers > > and different processes. > > > > I also expect that i915 will loose implicit syncing in a few upcoming > > hw modes because explicit syncing is a more natural fit there. > > > > All that isn't about the discussion at hand imo since no matter what > > i915 needs to have on internal representation for a bit of gpu work, > > and afaics right now we don't have that. With this patch android > > syncpts use Maarten's fences internally, but I can't freely exchange > > one for the other. So in i915 I still expect to get stuck with both of > > them, which is one too many. > > > > The other issue (and I haven't dug into details that much) I have with > > syncpts are some of the interface choices. Apparently you can commit a > > fence after creation (or at least the hw composer interface works like > > that) which means userspace can construct deadlocks with android > > syncpts if I'm not super careful in my driver. I haven't seen any > > generic code to do that, so I presume everyone just blindly trusts > > surface-flinger to not do that. Speaks of the average quality of an > > android gfx driver if the kernel is less trusted than the compositor > > in userspace ... > > Android sync is designed not to allow userspace to deadlock the > kernel, a sync_pt should only get created by the kernel when it has > received a chunk of work that it expects to complete in the near > future. The CONFIG_SW_SYNC_USER driver violates that by allowing > userspace to create and signal arbitrary sync points, but that is > intended only for testing sync. Ok, that makes sense. As long as we sufficiently taint the kernel and hide the sw_sync framework we should be good. I was confused by the hw composer interface spec which seemed to suggest that the fences for a screen update completion should be attached before surfaceflinger commits the state. But I never looked at an implemention so guess that impression is wrong. > > There's a few other things like exposing timestamps (which are tricky > > to do right, our driver is littered with wrong attempts) and other > > details. > > Timestamps are necessary for vsync synchronization to reduce the frame > latency. I'm not against timestamps (we have them for drm vblank events too after all), just would like for them to be optional. And we need to give userspace very clear indication which hw clock the timestamp was based on (or whether we're using the clock_monotonic system clock) to make sure debug and profiling tools can properly align things. Because hw clocks always get out of sync. Last time I've looked at the syncpt ioctls
Re: [PATCH 1/1 v2] media: dib9000: avoid out of bound access
On Thu, Jun 19, 2014 at 7:49 AM, Heinrich Schuchardt wrote: > This updated patch also fixes out of bound access to b[]. > > In dib9000_risc_apb_access_write() an out of bound access to mb[]. > > The current test to avoid out of bound access to mb[] is insufficient. > For len = 19 non-existent mb[10] will be accessed. > > For odd values of len b[] is accessed out of bounds. > > For large values of len an of bound access to mb[] may occur in > dib9000_mbx_send_attr. > > Signed-off-by: Heinrich Schuchardt > --- > drivers/media/dvb-frontends/dib9000.c | 13 + > 1 file changed, 9 insertions(+), 4 deletions(-) > > diff --git a/drivers/media/dvb-frontends/dib9000.c > b/drivers/media/dvb-frontends/dib9000.c > index e540cfb..f75dec4 100644 > --- a/drivers/media/dvb-frontends/dib9000.c > +++ b/drivers/media/dvb-frontends/dib9000.c > @@ -1040,13 +1040,18 @@ static int dib9000_risc_apb_access_write(struct > dib9000_state *state, u32 addres > if (address >= 1024 || !state->platform.risc.fw_is_running) > return -EINVAL; > > + if (len > 18) > + return -EINVAL; > + > /* dprintk( "APB access thru wr fw %d %x", address, attribute); */ > > - mb[0] = (unsigned short)address; > - for (i = 0; i < len && i < 20; i += 2) > - mb[1 + (i / 2)] = (b[i] << 8 | b[i + 1]); > + mb[0] = (u16)address; > + for (i = 0; i + 1 < len; i += 2) > + mb[1 + i / 2] = b[i] << 8 | b[i + 1]; > + if (len & 1) > + mb[1 + len / 2] = b[len - 1] << 8; > > - dib9000_mbx_send_attr(state, OUT_MSG_BRIDGE_APB_W, mb, 1 + len / 2, > attribute); > + dib9000_mbx_send_attr(state, OUT_MSG_BRIDGE_APB_W, mb, (3 + len) / 2, > attribute); > return dib9000_mbx_get_message_attr(state, IN_MSG_END_BRIDGE_APB_RW, > mb, &s, attribute) == 1 ? 0 : -EINVAL; > } > > -- > 2.0.0 > That looks great, thanks! Reviewed-by: Kees Cook -Kees -- Kees Cook Chrome OS Security -- 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: [REPOST PATCH 4/8] android: convert sync to fence api, v5
op 19-06-14 17:22, Colin Cross schreef:
> On Wed, Jun 18, 2014 at 11:37 PM, Daniel Vetter wrote:
>> On Wed, Jun 18, 2014 at 06:15:56PM -0700, Greg KH wrote:
>>> On Wed, Jun 18, 2014 at 12:37:11PM +0200, Maarten Lankhorst wrote:
Just to show it's easy.
Android syncpoints can be mapped to a timeline. This removes the need
to maintain a separate api for synchronization. I've left the android
trace events in place, but the core fence events should already be
sufficient for debugging.
v2:
- Call fence_remove_callback in sync_fence_free if not all fences have
fired.
v3:
- Merge Colin Cross' bugfixes, and the android fence merge optimization.
v4:
- Merge with the upstream fixes.
v5:
- Fix small style issues pointed out by Thomas Hellstrom.
Signed-off-by: Maarten Lankhorst
Acked-by: John Stultz
---
drivers/staging/android/Kconfig |1
drivers/staging/android/Makefile |2
drivers/staging/android/sw_sync.c|6
drivers/staging/android/sync.c | 913
+++---
drivers/staging/android/sync.h | 79 ++-
drivers/staging/android/sync_debug.c | 247 +
drivers/staging/android/trace/sync.h | 12
7 files changed, 609 insertions(+), 651 deletions(-)
create mode 100644 drivers/staging/android/sync_debug.c
>>> With these changes, can we pull the android sync logic out of
>>> drivers/staging/ now?
>> Afaik the google guys never really looked at this and acked it. So I'm not
>> sure whether they'll follow along. The other issue I have as the
>> maintainer of gfx driver is that I don't want to implement support for two
>> different sync object primitives (once for dma-buf and once for android
>> syncpts), and my impression thus far has been that even with this we're
>> not there.
> We have tested these patches to use dma fences to back the android
> sync driver and not found any major issues. However, my understanding
> is that dma fences are designed for implicit sync, and explicit sync
> through the android sync driver is bolted on the side to share code.
> Android is not moving away from explicit sync, but we do wrap all of
> our userspace sync accesses through libsync
> (https://android.googlesource.com/platform/system/core/+/master/libsync/sync.c,
> ignore the sw_sync parts), so if the kernel supported a slightly
> different userspace explicit sync interface we could adapt to it
> fairly easily. All we require is that individual kernel drivers need
> to be able to accept work alongisde an fd to wait on, and to return an
> fd that will signal when the work is done, and that userspace has some
> way to merge two of those fds, wait on an fd, and get some debugging
> info from an fd. However, this patch set doesn't do that, it has no
> way to export a dma fence as an fd except through the android sync
> driver, so it is not yet ready to fully replace android sync.
>
Dma fences can be exported as android fences, just didn't see a need for it
yet. :-)
To wait on all implicit fences attached to a dma-buf one could simply poll the
dma-buf directly,
or use something like a android userspace fence.
sync_fence_create takes a sync_pt as function argument, but I kept that to keep
source code
compatibility, not because it uses any sync_pt functions. Here's a patch to
create a userspace
fd for dma-fence instead of a android fence, applied on top of "android:
convert sync to fence api".
diff --git a/drivers/staging/android/sw_sync.c
b/drivers/staging/android/sw_sync.c
index a76db3ff87cb..afc3c63b0438 100644
--- a/drivers/staging/android/sw_sync.c
+++ b/drivers/staging/android/sw_sync.c
@@ -184,7 +184,7 @@ static long sw_sync_ioctl_create_fence(struct
sw_sync_timeline *obj,
}
data.name[sizeof(data.name) - 1] = '\0';
- fence = sync_fence_create(data.name, pt);
+ fence = sync_fence_create(data.name, &pt->base);
if (fence == NULL) {
sync_pt_free(pt);
err = -ENOMEM;
diff --git a/drivers/staging/android/sync.c b/drivers/staging/android/sync.c
index 70b09b5001ba..c89a6f954e41 100644
--- a/drivers/staging/android/sync.c
+++ b/drivers/staging/android/sync.c
@@ -188,7 +188,7 @@ static void fence_check_cb_func(struct fence *f, struct
fence_cb *cb)
}
/* TODO: implement a create which takes more that one sync_pt */
-struct sync_fence *sync_fence_create(const char *name, struct sync_pt *pt)
+struct sync_fence *sync_fence_create(const char *name, struct fence *pt)
{
struct sync_fence *fence;
@@ -199,10 +199,10 @@ struct sync_fence *sync_fence_create(const char *name,
struct sync_pt *pt)
fence->num_fences = 1;
atomic_set(&fence->status, 1);
- fence_get(&pt->base);
- fence->cbs[0].sync_pt = &pt->base;
+ fence_get(pt);
+ fence->cbs[0].sync_pt = pt;
fence->cbs[0].fence = fence;
- if (
Re: [REPOST PATCH 4/8] android: convert sync to fence api, v5
On Thu, Jun 19, 2014 at 5:28 AM, Daniel Vetter wrote: > On Thu, Jun 19, 2014 at 1:48 PM, Thierry Reding > wrote: >>> > With these changes, can we pull the android sync logic out of >>> > drivers/staging/ now? >>> >>> Afaik the google guys never really looked at this and acked it. So I'm not >>> sure whether they'll follow along. The other issue I have as the >>> maintainer of gfx driver is that I don't want to implement support for two >>> different sync object primitives (once for dma-buf and once for android >>> syncpts), and my impression thus far has been that even with this we're >>> not there. >>> >>> I'm trying to get our own android guys to upstream their i915 syncpts >>> support, but thus far I haven't managed to convince them to throw people's >>> time at this. >> >> This has been discussed a fair bit internally recently and some of our >> GPU experts have raised concerns that this may result in seriously >> degraded performance in our proprietary graphics stack. Now I don't care >> very much for the proprietary graphics stack, but by extension I would >> assume that the same restrictions are relevant for any open-source >> driver as well. >> >> I'm still trying to fully understand all the implications and at the >> same time get some of the people who raised concerns to join in this >> discussion. As I understand it the concern is mostly about explicit vs. >> implicit synchronization and having this mechanism in the kernel will >> implicitly synchronize all accesses to these buffers even in cases where >> it's not needed (read vs. write locks, etc.). In one particular instance >> it was even mentioned that this kind of implicit synchronization can >> lead to deadlocks in some use-cases (this was mentioned for Android >> compositing, but I suspect that the same may happen for Wayland or X >> compositors). > > Well the implicit fences here actually can't deadlock. That's the > entire point behind using ww mutexes. I've also heard tons of > complaints about implicit enforced syncing (especially from opencl > people), but in the end drivers and always expose unsynchronized > access for specific cases. We do that in i915 for upload buffers and > other fun stuff. This is about shared stuff across different drivers > and different processes. > > I also expect that i915 will loose implicit syncing in a few upcoming > hw modes because explicit syncing is a more natural fit there. > > All that isn't about the discussion at hand imo since no matter what > i915 needs to have on internal representation for a bit of gpu work, > and afaics right now we don't have that. With this patch android > syncpts use Maarten's fences internally, but I can't freely exchange > one for the other. So in i915 I still expect to get stuck with both of > them, which is one too many. > > The other issue (and I haven't dug into details that much) I have with > syncpts are some of the interface choices. Apparently you can commit a > fence after creation (or at least the hw composer interface works like > that) which means userspace can construct deadlocks with android > syncpts if I'm not super careful in my driver. I haven't seen any > generic code to do that, so I presume everyone just blindly trusts > surface-flinger to not do that. Speaks of the average quality of an > android gfx driver if the kernel is less trusted than the compositor > in userspace ... Android sync is designed not to allow userspace to deadlock the kernel, a sync_pt should only get created by the kernel when it has received a chunk of work that it expects to complete in the near future. The CONFIG_SW_SYNC_USER driver violates that by allowing userspace to create and signal arbitrary sync points, but that is intended only for testing sync. > There's a few other things like exposing timestamps (which are tricky > to do right, our driver is littered with wrong attempts) and other > details. Timestamps are necessary for vsync synchronization to reduce the frame latency. > Finally I've never seen anyone from google or any android product guy > push a real driver enabling for syncpts to upstream, and google itself > has a bit a history of constantly exchanging their gfx framework for > the next best thing. So I really doubt this is worthwhile to pursue in > upstream with our essentially eternal api guarantees. At least until > we see serious uptake from vendors and gfx driver guys. Unfortunately > the Intel android folks are no exception here and haven't pushed > anything like this in my direction yet at all. Despite multiple pokes > from my side. As far as I know, every SoC vendor that supports android is using sync now, but none of them have succeeded in pushing their drivers upstream for a variety of other reasons (interfaces only used by closed source userspaces, KMS/DRM vs ADF, ION, etc.). > So from my side I think we should move ahead with Maarten's work and > figure the android side out once there's real interest. As long as that doesn't involve r
Re: [REPOST PATCH 4/8] android: convert sync to fence api, v5
On Wed, Jun 18, 2014 at 11:37 PM, Daniel Vetter wrote: > On Wed, Jun 18, 2014 at 06:15:56PM -0700, Greg KH wrote: >> On Wed, Jun 18, 2014 at 12:37:11PM +0200, Maarten Lankhorst wrote: >> > Just to show it's easy. >> > >> > Android syncpoints can be mapped to a timeline. This removes the need >> > to maintain a separate api for synchronization. I've left the android >> > trace events in place, but the core fence events should already be >> > sufficient for debugging. >> > >> > v2: >> > - Call fence_remove_callback in sync_fence_free if not all fences have >> > fired. >> > v3: >> > - Merge Colin Cross' bugfixes, and the android fence merge optimization. >> > v4: >> > - Merge with the upstream fixes. >> > v5: >> > - Fix small style issues pointed out by Thomas Hellstrom. >> > >> > Signed-off-by: Maarten Lankhorst >> > Acked-by: John Stultz >> > --- >> > drivers/staging/android/Kconfig |1 >> > drivers/staging/android/Makefile |2 >> > drivers/staging/android/sw_sync.c|6 >> > drivers/staging/android/sync.c | 913 >> > +++--- >> > drivers/staging/android/sync.h | 79 ++- >> > drivers/staging/android/sync_debug.c | 247 + >> > drivers/staging/android/trace/sync.h | 12 >> > 7 files changed, 609 insertions(+), 651 deletions(-) >> > create mode 100644 drivers/staging/android/sync_debug.c >> >> With these changes, can we pull the android sync logic out of >> drivers/staging/ now? > > Afaik the google guys never really looked at this and acked it. So I'm not > sure whether they'll follow along. The other issue I have as the > maintainer of gfx driver is that I don't want to implement support for two > different sync object primitives (once for dma-buf and once for android > syncpts), and my impression thus far has been that even with this we're > not there. We have tested these patches to use dma fences to back the android sync driver and not found any major issues. However, my understanding is that dma fences are designed for implicit sync, and explicit sync through the android sync driver is bolted on the side to share code. Android is not moving away from explicit sync, but we do wrap all of our userspace sync accesses through libsync (https://android.googlesource.com/platform/system/core/+/master/libsync/sync.c, ignore the sw_sync parts), so if the kernel supported a slightly different userspace explicit sync interface we could adapt to it fairly easily. All we require is that individual kernel drivers need to be able to accept work alongisde an fd to wait on, and to return an fd that will signal when the work is done, and that userspace has some way to merge two of those fds, wait on an fd, and get some debugging info from an fd. However, this patch set doesn't do that, it has no way to export a dma fence as an fd except through the android sync driver, so it is not yet ready to fully replace android sync. > I'm trying to get our own android guys to upstream their i915 syncpts > support, but thus far I haven't managed to convince them to throw people's > time at this. > > It looks like a step into the right direction, but until I have the proof > in the form of i915 patches that I won't have to support 2 gfx fencing > frameworks I'm opposed to de-staging android syncpts. Ofc someone else > could do that too, but besides i915 I don't see a full-fledged (modeset > side only kinda doesn't count) upstream gfx driver shipping on android. > -Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch -- 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/1 v2] media: dib9000: avoid out of bound access
This updated patch also fixes out of bound access to b[]. In dib9000_risc_apb_access_write() an out of bound access to mb[]. The current test to avoid out of bound access to mb[] is insufficient. For len = 19 non-existent mb[10] will be accessed. For odd values of len b[] is accessed out of bounds. For large values of len an of bound access to mb[] may occur in dib9000_mbx_send_attr. Signed-off-by: Heinrich Schuchardt --- drivers/media/dvb-frontends/dib9000.c | 13 + 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/drivers/media/dvb-frontends/dib9000.c b/drivers/media/dvb-frontends/dib9000.c index e540cfb..f75dec4 100644 --- a/drivers/media/dvb-frontends/dib9000.c +++ b/drivers/media/dvb-frontends/dib9000.c @@ -1040,13 +1040,18 @@ static int dib9000_risc_apb_access_write(struct dib9000_state *state, u32 addres if (address >= 1024 || !state->platform.risc.fw_is_running) return -EINVAL; + if (len > 18) + return -EINVAL; + /* dprintk( "APB access thru wr fw %d %x", address, attribute); */ - mb[0] = (unsigned short)address; - for (i = 0; i < len && i < 20; i += 2) - mb[1 + (i / 2)] = (b[i] << 8 | b[i + 1]); + mb[0] = (u16)address; + for (i = 0; i + 1 < len; i += 2) + mb[1 + i / 2] = b[i] << 8 | b[i + 1]; + if (len & 1) + mb[1 + len / 2] = b[len - 1] << 8; - dib9000_mbx_send_attr(state, OUT_MSG_BRIDGE_APB_W, mb, 1 + len / 2, attribute); + dib9000_mbx_send_attr(state, OUT_MSG_BRIDGE_APB_W, mb, (3 + len) / 2, attribute); return dib9000_mbx_get_message_attr(state, IN_MSG_END_BRIDGE_APB_RW, mb, &s, attribute) == 1 ? 0 : -EINVAL; } -- 2.0.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: [RFC 2/2] gspca_kinect: add support for the depth stream
Hi,
On 06/04/2014 10:24 PM, Antonio Ospite wrote:
> Add support for the depth mode at 10bpp, use a command line parameter to
> switch mode.
>
> NOTE: this is just a proof-of-concept, the final implementation will
> have to expose two v4l2 devices, one for the video stream and one for
> the depth stream.
Thanks for the patch. If this is useful for some people I'm willing to
merge this until we've a better fix.
> Signed-off-by: Alexander Sosna
> Signed-off-by: Antonio Ospite
> ---
>
> For now a command line parameter called "depth_mode" is used to select which
> mode to activate when loading the driver, this is necessary because gspca is
> not quite ready to have a subdriver call gspca_dev_probe() multiple times.
>
> The problem seems to be that gspca assumes one video device per USB
> _interface_, and so it stores a pointer to gspca_dev as the interface
> private data: see usb_set_intfdata(intf, gspca_dev) in
> gspca_dev_probe2().
>
> If anyone feels brave (do a backup first, etc. etc.), hack the sd_probe()
> below to register both the devices: you will get the two v4l nodes and both
> streams will work OK, but the kernel will halt when you disconnect the device,
> i.e. some problem arises in gspca_disconnect() after the
> usb_get_intfdata(intf)
> call.
>
> I am still figuring out the details of the failure sequence, and I'll try to
> imagine a way to support the use case "multiple v4l devices on one USB
> interface", but this will take some more time.
I believe that support 2 devices would require separating the per video node /
stream data and global data into separate structs, and then refactoring
everything
so that we can have 2 streams on one gspca_dev. If you do this please make it
a patch-set with many small patches, rather then 1 or 2 very large patches.
And then in things like disconnect, loop over the streams and stop both,
unregister
both nodes, etc.
If you ever decide to add support for controls you will also need to think
about what
to do with those, but for now I guess you can just register all the controls on
the
first video-node/stream (which will be the only one for all devices except
kinect
devices, and the kinect code currently does not have controls.
Regards,
Hans
>
> Thanks,
>Antonio
>
> drivers/media/usb/gspca/kinect.c | 97
> +++-
> 1 file changed, 86 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/media/usb/gspca/kinect.c
> b/drivers/media/usb/gspca/kinect.c
> index 081f051..ce688d8 100644
> --- a/drivers/media/usb/gspca/kinect.c
> +++ b/drivers/media/usb/gspca/kinect.c
> @@ -36,6 +36,8 @@ MODULE_AUTHOR("Antonio Ospite ");
> MODULE_DESCRIPTION("GSPCA/Kinect Sensor Device USB Camera Driver");
> MODULE_LICENSE("GPL");
>
> +static bool depth_mode;
> +
> struct pkt_hdr {
> uint8_t magic[2];
> uint8_t pad;
> @@ -73,6 +75,14 @@ struct sd {
>
> #define FPS_HIGH 0x0100
>
> +static const struct v4l2_pix_format depth_camera_mode[] = {
> + {640, 480, V4L2_PIX_FMT_Y10BPACK, V4L2_FIELD_NONE,
> + .bytesperline = 640 * 10 / 8,
> + .sizeimage = 640 * 480 * 10 / 8,
> + .colorspace = V4L2_COLORSPACE_SRGB,
> + .priv = MODE_640x488 | FORMAT_Y10B},
> +};
> +
> static const struct v4l2_pix_format video_camera_mode[] = {
> {640, 480, V4L2_PIX_FMT_SGRBG8, V4L2_FIELD_NONE,
>.bytesperline = 640,
> @@ -219,7 +229,7 @@ static int write_register(struct gspca_dev *gspca_dev,
> uint16_t reg,
> }
>
> /* this function is called at probe time */
> -static int sd_config(struct gspca_dev *gspca_dev,
> +static int sd_config_video(struct gspca_dev *gspca_dev,
>const struct usb_device_id *id)
> {
> struct sd *sd = (struct sd *) gspca_dev;
> @@ -227,8 +237,7 @@ static int sd_config(struct gspca_dev *gspca_dev,
>
> sd->cam_tag = 0;
>
> - /* Only video stream is supported for now,
> - * which has stream flag = 0x80 */
> + /* video has stream flag = 0x80 */
> sd->stream_flag = 0x80;
>
> cam = &gspca_dev->cam;
> @@ -245,6 +254,26 @@ static int sd_config(struct gspca_dev *gspca_dev,
> return 0;
> }
>
> +static int sd_config_depth(struct gspca_dev *gspca_dev,
> + const struct usb_device_id *id)
> +{
> + struct sd *sd = (struct sd *) gspca_dev;
> + struct cam *cam;
> +
> + sd->cam_tag = 0;
> +
> + cam = &gspca_dev->cam;
> +
> + /* depth has stream flag = 0x70 */
> + sd->stream_flag = 0x70;
> + cam->cam_mode = depth_camera_mode;
> + cam->nmodes = ARRAY_SIZE(depth_camera_mode);
> +
> + gspca_dev->xfer_ep_index = 1;
> +
> + return 0;
> +}
> +
> /* this function is called at probe and resume time */
> static int sd_init(struct gspca_dev *gspca_dev)
> {
> @@ -253,7 +282,7 @@ static int sd_init(struct gspca_dev *gspca_dev)
> return 0;
> }
>
> -static int sd_start(struct gspca_dev *gspca_dev)
> +static int sd_start_video(struct gspca_dev *gspca_dev)
Re: [RFC 1/2] gspca: provide a mechanism to select a specific transfer endpoint
Hi Antonio,
Thanks for working on this.
On 06/04/2014 10:24 PM, Antonio Ospite wrote:
> Add a xfer_ep_index field to struct gspca_dev, and change alt_xfer() so
> that it accepts a parameter which represents a specific endpoint to look
> for.
>
> If a subdriver wants to specify a value for gspca_dev->xfer_ep_index it
> can do that in its sd_config() callback.
>
> Signed-off-by: Antonio Ospite
> ---
>
> I am not sure if it is OK to specify an endpoint _index_ or if it would be
> better to specify the endpoint address directly (in Kinect 0x81 is for video
> data and 0x82 is for depth data).
>
> Hans, any comment on that?
I think it would be better to use the endpoint address directly for this,
relying on the order in which the endpoints are listed in the descriptor
feels wrong to me.
Other then that this patch looks good.
Regards,
Hans
>
> Thanks,
>Antonio
>
> drivers/media/usb/gspca/gspca.c | 20 ++--
> drivers/media/usb/gspca/gspca.h | 1 +
> 2 files changed, 15 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/media/usb/gspca/gspca.c b/drivers/media/usb/gspca/gspca.c
> index f3a7ace..7e5226c 100644
> --- a/drivers/media/usb/gspca/gspca.c
> +++ b/drivers/media/usb/gspca/gspca.c
> @@ -603,10 +603,13 @@ static void gspca_stream_off(struct gspca_dev
> *gspca_dev)
> }
>
> /*
> - * look for an input transfer endpoint in an alternate setting
> + * look for an input transfer endpoint in an alternate setting.
> + *
> + * If xfer_ep_index is negative, return the first valid one found, otherwise
> + * look for exactly the one in position xfer_ep.
> */
> static struct usb_host_endpoint *alt_xfer(struct usb_host_interface *alt,
> - int xfer)
> + int xfer, int xfer_ep_index)
> {
> struct usb_host_endpoint *ep;
> int i, attr;
> @@ -616,8 +619,10 @@ static struct usb_host_endpoint *alt_xfer(struct
> usb_host_interface *alt,
> attr = ep->desc.bmAttributes & USB_ENDPOINT_XFERTYPE_MASK;
> if (attr == xfer
> && ep->desc.wMaxPacketSize != 0
> - && usb_endpoint_dir_in(&ep->desc))
> + && usb_endpoint_dir_in(&ep->desc)
> + && (xfer_ep_index < 0 || i == xfer_ep_index)) {
> return ep;
> + }
> }
> return NULL;
> }
> @@ -689,7 +694,7 @@ static int build_isoc_ep_tb(struct gspca_dev *gspca_dev,
> found = 0;
> for (j = 0; j < nbalt; j++) {
> ep = alt_xfer(&intf->altsetting[j],
> - USB_ENDPOINT_XFER_ISOC);
> + USB_ENDPOINT_XFER_ISOC,
> gspca_dev->xfer_ep_index);
> if (ep == NULL)
> continue;
> if (ep->desc.bInterval == 0) {
> @@ -862,7 +867,8 @@ static int gspca_init_transfer(struct gspca_dev
> *gspca_dev)
> /* if bulk or the subdriver forced an altsetting, get the endpoint */
> if (gspca_dev->alt != 0) {
> gspca_dev->alt--; /* (previous version compatibility) */
> - ep = alt_xfer(&intf->altsetting[gspca_dev->alt], xfer);
> + ep = alt_xfer(&intf->altsetting[gspca_dev->alt], xfer,
> + gspca_dev->xfer_ep_index);
> if (ep == NULL) {
> pr_err("bad altsetting %d\n", gspca_dev->alt);
> return -EIO;
> @@ -904,7 +910,8 @@ static int gspca_init_transfer(struct gspca_dev
> *gspca_dev)
> if (!gspca_dev->cam.no_urb_create) {
> PDEBUG(D_STREAM, "init transfer alt %d", alt);
> ret = create_urbs(gspca_dev,
> - alt_xfer(&intf->altsetting[alt], xfer));
> + alt_xfer(&intf->altsetting[alt], xfer,
> + gspca_dev->xfer_ep_index));
> if (ret < 0) {
> destroy_urbs(gspca_dev);
> goto out;
> @@ -2030,6 +2037,7 @@ int gspca_dev_probe2(struct usb_interface *intf,
> }
> gspca_dev->dev = dev;
> gspca_dev->iface = intf->cur_altsetting->desc.bInterfaceNumber;
> + gspca_dev->xfer_ep_index = -1;
>
> /* check if any audio device */
> if (dev->actconfig->desc.bNumInterfaces != 1) {
> diff --git a/drivers/media/usb/gspca/gspca.h b/drivers/media/usb/gspca/gspca.h
> index 300642d..92317af 100644
> --- a/drivers/media/usb/gspca/gspca.h
> +++ b/drivers/media/usb/gspca/gspca.h
> @@ -205,6 +205,7 @@ struct gspca_dev {
> char memory;/* memory type (V4L2_MEMORY_xxx) */
> __u8 iface; /* USB interface number */
> __u8 alt; /* USB alternate setting */
> + int xfer_ep_index; /* index of the USB transfer endpoint */
>
Re: [REPOST PATCH 1/8] fence: dma-buf cross-device synchronization (v17)
On Wed, Jun 18, 2014 at 9:13 PM, Greg KH wrote:
> On Wed, Jun 18, 2014 at 12:36:54PM +0200, Maarten Lankhorst wrote:
>> +#define CREATE_TRACE_POINTS
>> +#include
>> +
>> +EXPORT_TRACEPOINT_SYMBOL(fence_annotate_wait_on);
>> +EXPORT_TRACEPOINT_SYMBOL(fence_emit);
>
> Are you really willing to live with these as tracepoints for forever?
> What is the use of them in debugging? Was it just for debugging the
> fence code, or for something else?
>
>> +/**
>> + * fence_context_alloc - allocate an array of fence contexts
>> + * @num: [in]amount of contexts to allocate
>> + *
>> + * This function will return the first index of the number of fences
>> allocated.
>> + * The fence context is used for setting fence->context to a unique number.
>> + */
>> +unsigned fence_context_alloc(unsigned num)
>> +{
>> + BUG_ON(!num);
>> + return atomic_add_return(num, &fence_context_counter) - num;
>> +}
>> +EXPORT_SYMBOL(fence_context_alloc);
>
> EXPORT_SYMBOL_GPL()? Same goes for all of the exports in here.
> Traditionally all of the driver core exports have been with this
> marking, any objection to making that change here as well?
tbh, I prefer EXPORT_SYMBOL().. well, I'd prefer even more if there
wasn't even a need for EXPORT_SYMBOL_GPL(), but sadly it is a fact of
life. We already went through this debate once with dma-buf. We
aren't going to change $evil_vendor's mind about non-gpl modules. The
only result will be a more flugly convoluted solution (ie. use syncpt
EXPORT_SYMBOL() on top of fence EXPORT_SYMBOL_GPL()) just as a
workaround, with the result that no-one benefits.
>> +int __fence_signal(struct fence *fence)
>> +{
>> + struct fence_cb *cur, *tmp;
>> + int ret = 0;
>> +
>> + if (WARN_ON(!fence))
>> + return -EINVAL;
>> +
>> + if (!ktime_to_ns(fence->timestamp)) {
>> + fence->timestamp = ktime_get();
>> + smp_mb__before_atomic();
>> + }
>> +
>> + if (test_and_set_bit(FENCE_FLAG_SIGNALED_BIT, &fence->flags)) {
>> + ret = -EINVAL;
>> +
>> + /*
>> + * we might have raced with the unlocked fence_signal,
>> + * still run through all callbacks
>> + */
>> + } else
>> + trace_fence_signaled(fence);
>> +
>> + list_for_each_entry_safe(cur, tmp, &fence->cb_list, node) {
>> + list_del_init(&cur->node);
>> + cur->func(fence, cur);
>> + }
>> + return ret;
>> +}
>> +EXPORT_SYMBOL(__fence_signal);
>
> Don't export a function with __ in front of it, you are saying that an
> internal function is somehow "valid" for everyone else to call? Why?
> You aren't even documenting the function, so who knows how to use it?
fwiw, the __ versions appear to mainly be concessions for android
syncpt. That is the only user outside of fence.c, and it should stay
that way.
>> +/**
>> + * fence_signal - signal completion of a fence
>> + * @fence: the fence to signal
>> + *
>> + * Signal completion for software callbacks on a fence, this will unblock
>> + * fence_wait() calls and run all the callbacks added with
>> + * fence_add_callback(). Can be called multiple times, but since a fence
>> + * can only go from unsignaled to signaled state, it will only be effective
>> + * the first time.
>> + */
>> +int fence_signal(struct fence *fence)
>> +{
>> + unsigned long flags;
>> +
>> + if (!fence)
>> + return -EINVAL;
>> +
>> + if (!ktime_to_ns(fence->timestamp)) {
>> + fence->timestamp = ktime_get();
>> + smp_mb__before_atomic();
>> + }
>> +
>> + if (test_and_set_bit(FENCE_FLAG_SIGNALED_BIT, &fence->flags))
>> + return -EINVAL;
>> +
>> + trace_fence_signaled(fence);
>> +
>> + if (test_bit(FENCE_FLAG_ENABLE_SIGNAL_BIT, &fence->flags)) {
>> + struct fence_cb *cur, *tmp;
>> +
>> + spin_lock_irqsave(fence->lock, flags);
>> + list_for_each_entry_safe(cur, tmp, &fence->cb_list, node) {
>> + list_del_init(&cur->node);
>> + cur->func(fence, cur);
>> + }
>> + spin_unlock_irqrestore(fence->lock, flags);
>> + }
>> + return 0;
>> +}
>> +EXPORT_SYMBOL(fence_signal);
>
> So, why should I use this over __fence_signal? What is the difference?
> Am I missing some documentation somewhere?
>
>> +void release_fence(struct kref *kref)
>> +{
>> + struct fence *fence =
>> + container_of(kref, struct fence, refcount);
>> +
>> + trace_fence_destroy(fence);
>> +
>> + BUG_ON(!list_empty(&fence->cb_list));
>> +
>> + if (fence->ops->release)
>> + fence->ops->release(fence);
>> + else
>> + kfree(fence);
>> +}
>> +EXPORT_SYMBOL(release_fence);
>
> fence_release() makes it more unified with the other functions in this
> file, right?
>
>> +/**
>> + * fence_default_wait - default sleep until the fence gets signaled
>> + * or until timeout elapses
Re: [REPOST PATCH 4/8] android: convert sync to fence api, v5
On Thu, Jun 19, 2014 at 1:48 PM, Thierry Reding wrote: >> > With these changes, can we pull the android sync logic out of >> > drivers/staging/ now? >> >> Afaik the google guys never really looked at this and acked it. So I'm not >> sure whether they'll follow along. The other issue I have as the >> maintainer of gfx driver is that I don't want to implement support for two >> different sync object primitives (once for dma-buf and once for android >> syncpts), and my impression thus far has been that even with this we're >> not there. >> >> I'm trying to get our own android guys to upstream their i915 syncpts >> support, but thus far I haven't managed to convince them to throw people's >> time at this. > > This has been discussed a fair bit internally recently and some of our > GPU experts have raised concerns that this may result in seriously > degraded performance in our proprietary graphics stack. Now I don't care > very much for the proprietary graphics stack, but by extension I would > assume that the same restrictions are relevant for any open-source > driver as well. > > I'm still trying to fully understand all the implications and at the > same time get some of the people who raised concerns to join in this > discussion. As I understand it the concern is mostly about explicit vs. > implicit synchronization and having this mechanism in the kernel will > implicitly synchronize all accesses to these buffers even in cases where > it's not needed (read vs. write locks, etc.). In one particular instance > it was even mentioned that this kind of implicit synchronization can > lead to deadlocks in some use-cases (this was mentioned for Android > compositing, but I suspect that the same may happen for Wayland or X > compositors). Well the implicit fences here actually can't deadlock. That's the entire point behind using ww mutexes. I've also heard tons of complaints about implicit enforced syncing (especially from opencl people), but in the end drivers and always expose unsynchronized access for specific cases. We do that in i915 for upload buffers and other fun stuff. This is about shared stuff across different drivers and different processes. I also expect that i915 will loose implicit syncing in a few upcoming hw modes because explicit syncing is a more natural fit there. All that isn't about the discussion at hand imo since no matter what i915 needs to have on internal representation for a bit of gpu work, and afaics right now we don't have that. With this patch android syncpts use Maarten's fences internally, but I can't freely exchange one for the other. So in i915 I still expect to get stuck with both of them, which is one too many. The other issue (and I haven't dug into details that much) I have with syncpts are some of the interface choices. Apparently you can commit a fence after creation (or at least the hw composer interface works like that) which means userspace can construct deadlocks with android syncpts if I'm not super careful in my driver. I haven't seen any generic code to do that, so I presume everyone just blindly trusts surface-flinger to not do that. Speaks of the average quality of an android gfx driver if the kernel is less trusted than the compositor in userspace ... There's a few other things like exposing timestamps (which are tricky to do right, our driver is littered with wrong attempts) and other details. Finally I've never seen anyone from google or any android product guy push a real driver enabling for syncpts to upstream, and google itself has a bit a history of constantly exchanging their gfx framework for the next best thing. So I really doubt this is worthwhile to pursue in upstream with our essentially eternal api guarantees. At least until we see serious uptake from vendors and gfx driver guys. Unfortunately the Intel android folks are no exception here and haven't pushed anything like this in my direction yet at all. Despite multiple pokes from my side. So from my side I think we should move ahead with Maarten's work and figure the android side out once there's real interest. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch -- 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] drivers/media/rc/ir-nec-decode : add toggle feature
On 2014-06-19 11:05, David Härdeman wrote: > On Thu, Jun 19, 2014 at 10:25:29AM +0200, Niels Laukens wrote: >> Made the distinction between repeated key presses, and a single long >> press. The NEC-protocol does not have a toggle-bit (cfr RC5/RC6), but >> has specific repeat-codes. > > Not all NEC remotes use repeat codes. Some just transmit the full code > at fixed intervals...IIRC, Pioneer remotes is (was?) one example... A way to cover this, is to make this mechanism optional, and auto-activate as soon as a repeat code is seen. But that will only work reliably with a single (type of) remote per system. Is this a better solution? Niels -- 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: [REPOST PATCH 4/8] android: convert sync to fence api, v5
On Thu, Jun 19, 2014 at 08:37:27AM +0200, Daniel Vetter wrote: > On Wed, Jun 18, 2014 at 06:15:56PM -0700, Greg KH wrote: > > On Wed, Jun 18, 2014 at 12:37:11PM +0200, Maarten Lankhorst wrote: > > > Just to show it's easy. > > > > > > Android syncpoints can be mapped to a timeline. This removes the need > > > to maintain a separate api for synchronization. I've left the android > > > trace events in place, but the core fence events should already be > > > sufficient for debugging. > > > > > > v2: > > > - Call fence_remove_callback in sync_fence_free if not all fences have > > > fired. > > > v3: > > > - Merge Colin Cross' bugfixes, and the android fence merge optimization. > > > v4: > > > - Merge with the upstream fixes. > > > v5: > > > - Fix small style issues pointed out by Thomas Hellstrom. > > > > > > Signed-off-by: Maarten Lankhorst > > > Acked-by: John Stultz > > > --- > > > drivers/staging/android/Kconfig |1 > > > drivers/staging/android/Makefile |2 > > > drivers/staging/android/sw_sync.c|6 > > > drivers/staging/android/sync.c | 913 > > > +++--- > > > drivers/staging/android/sync.h | 79 ++- > > > drivers/staging/android/sync_debug.c | 247 + > > > drivers/staging/android/trace/sync.h | 12 > > > 7 files changed, 609 insertions(+), 651 deletions(-) > > > create mode 100644 drivers/staging/android/sync_debug.c > > > > With these changes, can we pull the android sync logic out of > > drivers/staging/ now? > > Afaik the google guys never really looked at this and acked it. So I'm not > sure whether they'll follow along. The other issue I have as the > maintainer of gfx driver is that I don't want to implement support for two > different sync object primitives (once for dma-buf and once for android > syncpts), and my impression thus far has been that even with this we're > not there. > > I'm trying to get our own android guys to upstream their i915 syncpts > support, but thus far I haven't managed to convince them to throw people's > time at this. This has been discussed a fair bit internally recently and some of our GPU experts have raised concerns that this may result in seriously degraded performance in our proprietary graphics stack. Now I don't care very much for the proprietary graphics stack, but by extension I would assume that the same restrictions are relevant for any open-source driver as well. I'm still trying to fully understand all the implications and at the same time get some of the people who raised concerns to join in this discussion. As I understand it the concern is mostly about explicit vs. implicit synchronization and having this mechanism in the kernel will implicitly synchronize all accesses to these buffers even in cases where it's not needed (read vs. write locks, etc.). In one particular instance it was even mentioned that this kind of implicit synchronization can lead to deadlocks in some use-cases (this was mentioned for Android compositing, but I suspect that the same may happen for Wayland or X compositors). Thierry pgpJIsKfAjjEU.pgp Description: PGP signature
Re: [RFC PATCH] [media] mt9v032: Add support for mt9v022 and mt9v024
Hi Laurent, Philipp, Sorry for a late reply. On Thu, 5 Jun 2014, Laurent Pinchart wrote: > Hi Philipp, > > On Tuesday 03 June 2014 11:30:31 Philipp Zabel wrote: > > Am Mittwoch, den 28.05.2014, 16:44 +0200 schrieb Laurent Pinchart: > > > If you had submitted an entirely new driver for a sensor already supported > > > by an soc-camera sensor driver, I would have told you to fix the problem > > > on soc- camera side. As you're only expanding hardware support for an > > > existing driver, it's hard to nack your patch in all fairness :-) I will > > > thus not veto option 2, even though I would prefer if we fixed the > > > problem once and for all. > > > > > > > > > I'm ok with either 1 or 3, whereas 3 is > > > > > > more difficult than 1. > > > > > > > > > > This topic has been discussed over and over. It indeed "just" requires > > > > > someone to do it, although it might be more complex than that sounds. > > > > > > > > > > We need to fix the infrastructure to make sensor drivers completely > > > > > unaware of soc-camera. This isn't about extending the mt9v022 driver > > > > > to work with non soc-camera hosts, it's about fixing soc-camera not to > > > > > require any change to sensor drivers. Philipp, if you have time to > > > > > work on that, we can discuss what needs to be done. > > > > What steps would need to be taken to make soc_camera work with the > > non-soc_camera drivers in drivers/media/i2c? > > Guennadi, what's the status of your work on this ? What remains to be done ? I just uploaded my last - unfinished - attempt to make an OMAP3 beagle-board work with an OV772x sensor to http://download.open-technology.de/testing/soc-camera-integration/ Patches can be pushed upstream, the .diff is, obviously, still a WiP. The work hasn't been finished, because O got some problems with the video, but it could well have been a problem with the specific set up, not with the conversion. Those patches were last used with a -next snapshot from 24.12.2013, so, they might not apply directly today, but soc-camera hasn't changed much since then, so, conflicts shouldn't be large or difficult to resolve. > The soc-camera core provides several helper functions for subdev drivers, and > expects the subdev drivers to implement bus configuration negotiation with > the > g_mbus_config and s_mbus_config subdev operations. > > If you look at the mt9v022 driver, the helper functions used are > > - soc_camera_limit_side > - soc_mbus_get_fmtdesc > - soc_camera_set_power > - soc_camera_apply_board_flags > > The first two functions are general purpose helpers that could be > standardized > and moved to the v4l core, or even left in soc-camera for now as they don't > depend on the bridge driver being compatible with soc-camera. > > The last two functions are mostly self-contained as well, but depend on the > I2C sensor device using a soc-camera structure (soc_camera_subdev_desc) for > its platform data. This isn't a requirement. This is just how this specific driver chooses to have its platform data. The omap3-isp-ov772x.diff at the above location shows examples, how this can be changed. > That structure contains field that are common across many > sensors, as well as a pointer to a sensor-specific platform data structure. > The common structure is then passed to various helper functions by the sensor > driver. > > That's something I would like to see being changed, the sensor should use a > custom structure for its platform data. This is done in those patches. > If the sensor drivers wants to use the > soc-camera helpers that don't depend on the host-side of soc-camera, it could > then embed a soc-camera platform data structure inside its own platform data > structure, and pass a pointer to that embedded structure to the soc-camera > helpers. Exactly, this is possible now too, just respective drivers and platforms have to be changed. > Another point that needs to be fixed is that soc-camera performs several > initialization steps for the sensor before probing it, such as calling > devm_regulator_bulk_get() to retrieve the sensor regulators. This is only done in the legacy mode. In async / DT mode I2C drivers have to call soc_camera_power_init() themselves. > Those steps > require the sensor to use the struct soc_camera_subdev_desc as platform data. > This should be changed as well, sensor drivers should call a soc-camera > helper > function explicitly from their probe function to perform the same task. > > I don't remember the details of how soc-camera handles [gs]_mbus_config, but > changes might be needed there as well. I think that's entirely up to specific camera host drivers. > That's more or less what I see needing to be fixed. Guennadi, please feel > free > to correct me. AFAICS, the current state has almost no restrictions at the soc-camera core level. Most of the code at the above link deals with drivers and platforms, just a couple of tweaks were require
Re: [PATCH 1/2] drivers/media/rc/ir-nec-decode : add toggle feature
On Thu, Jun 19, 2014 at 10:25:29AM +0200, Niels Laukens wrote: >Made the distinction between repeated key presses, and a single long >press. The NEC-protocol does not have a toggle-bit (cfr RC5/RC6), but >has specific repeat-codes. Not all NEC remotes use repeat codes. Some just transmit the full code at fixed intervals...IIRC, Pioneer remotes is (was?) one example... >This patch identifies a repeat code, and skips the scancode calculations >and the rc_keydown() in that case. In the case of a full code, it makes >sure that the rc_keydown() is regarded as a new event by using the >toggle feature. -- 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 RESEND] libv4lconvert: Fix a regression when converting from Y10B
Hi, On 06/18/2014 04:40 PM, Antonio Ospite wrote: > On Wed, 18 Jun 2014 15:59:13 +0200 > Hans de Goede wrote: > >> Hi, >> >> On 06/18/2014 03:23 PM, Antonio Ospite wrote: >>> On Wed, 18 Jun 2014 13:46:10 +0200 >>> Hans de Goede wrote: >>> Hi, On 06/18/2014 01:43 PM, Hans de Goede wrote: > Hi, > > On 06/16/2014 05:00 PM, Antonio Ospite wrote: > [...] > Why print a message here at all in the != 0 case? In the old code before > commit > efc29f1764 you did not print an error when v4lconvert_y10b_to_... failed, > so > I assume that that already does a V4LCONVERT_ERR in that case. So why do > it a > second time with a less precise error message here? > >>> >>> The one from v4lconvert_oom_error(), yes, which is generic, it does not >>> tell _where_ the failure was. >>> > So I believe that the proper fix would be to just remove the entire block > instead > of flipping the test and keeping the V4LCONVERT_ERR. Please send a new > version > with this fixed, then I'll merge it asap. Scrap that, I decided I might just as well fix this bit myself, so I've just pushed an updated patch completely removing the second check from the V4L2_PIX_FMT_Y10BPACK case. >>> >>> The rationale behind leaving the message was: >>> 1. The conversion routines are called even in the case of short >>> frames (BTW that is true for any format, not just for Y10B). >>> 2. The conversion routines from Y10B are not "in place", they >>> allocate a temporary buffer, so they may fail themselves. >> >> Right, and this already does a V4LCONVERT_ERR, which will override any >> error msg stored earlier. >> > > I see now, I was overlooking how V4LCONVERT_ERR() works. > >>> with this in mind I saw the second message as an _additional_ error >>> indication to the user (useful in case of short frame _and_ conversion >>> failure) rather than a less precise one. However, you are right that >>> this additional error message was not in the original code before >>> efc29f1764, so your patch is perfectly fine by me. >>> >>> Thanks for merging it. >>> >>> BTW, comments about 1.? >>> What's the idea behind calling the conversion routines even for short >>> frames? >> >> For short frames the higher layer (libv4l2) will retry up to 3 times and then >> just return whatever it did get. The src_size is the amount of available >> bytes >> in the source buffer, the actual source buffer is pre-allocated and is always >> large enough, so in case of 3 consecutive short frames we convert whatever we >> did get + whatever data there was already in the buffer for the rest of the >> frame and return that to the user. >> >> This is useful since if the vsync timing is off between bridge and sensor, >> we often miss some lines at the bottom. So by converting what ever we do get >> we >> end up returning a frame with a mostly complete picture + 2 lines of garbage >> at >> the bottom at 1/3th of the framerate because of the retries. >> >> Ideally this would never happen, but it does and in this case actually >> showing >> the broken picture and allowing the user to take a screenshot of this and >> attach it to a bug report makes things a whole lot easier to debug. And in >> this >> case the camera is even still somewhat usable by the user this way. >> >> Likewise in other cases where the driver consistently feeds us short frames, >> it can be quite helpful to actually see the contents of the short frame >> for debugging purposes. >> >> Regards, >> >> Hans >> > > Thanks for the explanation. > > Ciao, >Antonio > > P.S. can we please have commit fff7e0eaae9734aa1f0a4e0fadef0d8c5c41b1e8 > cherry-picked in the stable-1.0 branch? cherry-picked and pushed. Regards, Hans -- 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] drivers/media/rc/ir-nec-decode : add toggle feature
>From 83aa9f9fa0eaf9eb8005af49f5ce93d24a0b9f2e Mon Sep 17 00:00:00 2001
From: Niels Laukens
Date: Thu, 19 Jun 2014 10:05:11 +0200
Subject: [PATCH 1/2] drivers/media/rc/ir-nec-decode : add toggle feature (1/2)
Made the distinction between repeated key presses, and a single long
press. The NEC-protocol does not have a toggle-bit (cfr RC5/RC6), but
has specific repeat-codes.
This patch identifies a repeat code, and skips the scancode calculations
and the rc_keydown() in that case. In the case of a full code, it makes
sure that the rc_keydown() is regarded as a new event by using the
toggle feature.
Signed-off-by: Niels Laukens
---
drivers/media/rc/ir-nec-decoder.c | 7 ++-
drivers/media/rc/rc-core-priv.h | 1 +
2 files changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/media/rc/ir-nec-decoder.c
b/drivers/media/rc/ir-nec-decoder.c
index 35c42e5..1f2482a 100644
--- a/drivers/media/rc/ir-nec-decoder.c
+++ b/drivers/media/rc/ir-nec-decoder.c
@@ -79,6 +79,7 @@ static int ir_nec_decode(struct rc_dev *dev, struct
ir_raw_event ev)
break;
data->count = 0;
+ data->nec_repeat = false;
data->state = STATE_HEADER_SPACE;
return 0;
@@ -93,6 +94,7 @@ static int ir_nec_decode(struct rc_dev *dev, struct
ir_raw_event ev)
if (!dev->keypressed) {
IR_dprintk(1, "Discarding last key repeat:
event after key up\n");
} else {
+ data->nec_repeat = true;
rc_repeat(dev);
IR_dprintk(1, "Repeat last key\n");
data->state = STATE_TRAILER_PULSE;
@@ -158,6 +160,7 @@ static int ir_nec_decode(struct rc_dev *dev, struct
ir_raw_event ev)
if (!geq_margin(ev.duration, NEC_TRAILER_SPACE, NEC_UNIT / 2))
break;
+ if (!data->nec_repeat) {
address = bitrev8((data->bits >> 24) & 0xff);
not_address = bitrev8((data->bits >> 16) & 0xff);
command = bitrev8((data->bits >> 8) & 0xff);
@@ -189,7 +192,9 @@ static int ir_nec_decode(struct rc_dev *dev, struct
ir_raw_event ev)
if (data->is_nec_x)
data->necx_repeat = true;
- rc_keydown(dev, scancode, 0);
+ rc_keydown(dev, scancode, !dev->last_toggle);
+ }
+
data->state = STATE_INACTIVE;
return 0;
}
diff --git a/drivers/media/rc/rc-core-priv.h b/drivers/media/rc/rc-core-priv.h
index da536c9..37f3b74 100644
--- a/drivers/media/rc/rc-core-priv.h
+++ b/drivers/media/rc/rc-core-priv.h
@@ -49,6 +49,7 @@ struct ir_raw_event_ctrl {
u32 bits;
bool is_nec_x;
bool necx_repeat;
+ bool nec_repeat;
} nec;
struct rc5_dec {
int state;
--
1.8.5.2 (Apple Git-48)
--
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] drivers/media/rc/ir-nec-decode : add toggle feature (2/2)
>From a8fca4a8c37cb35ea4527a708a6894745f81c661 Mon Sep 17 00:00:00 2001
From: Niels Laukens
Date: Thu, 19 Jun 2014 10:06:00 +0200
Subject: [PATCH 2/2] drivers/media/rc/ir-nec-decode : add toggle feature (2/2)
Fixes indentation. Kept as separate patch to keep patch 1/2 more to the point.
Signed-off-by: Niels Laukens
---
drivers/media/rc/ir-nec-decoder.c | 61 ---
1 file changed, 32 insertions(+), 29 deletions(-)
diff --git a/drivers/media/rc/ir-nec-decoder.c
b/drivers/media/rc/ir-nec-decoder.c
index 1f2482a..ff8b5d7 100644
--- a/drivers/media/rc/ir-nec-decoder.c
+++ b/drivers/media/rc/ir-nec-decoder.c
@@ -161,38 +161,41 @@ static int ir_nec_decode(struct rc_dev *dev, struct
ir_raw_event ev)
break;
if (!data->nec_repeat) {
- address = bitrev8((data->bits >> 24) & 0xff);
- not_address = bitrev8((data->bits >> 16) & 0xff);
- command = bitrev8((data->bits >> 8) & 0xff);
- not_command = bitrev8((data->bits >> 0) & 0xff);
-
- if ((command ^ not_command) != 0xff) {
- IR_dprintk(1, "NEC checksum error: received 0x%08x\n",
- data->bits);
- send_32bits = true;
- }
+ address = bitrev8((data->bits >> 24) & 0xff);
+ not_address = bitrev8((data->bits >> 16) & 0xff);
+ command = bitrev8((data->bits >> 8) & 0xff);
+ not_command = bitrev8((data->bits >> 0) & 0xff);
+
+ if ((command ^ not_command) != 0xff) {
+ IR_dprintk(1, "NEC checksum error: received
0x%08x\n",
+ data->bits);
+ send_32bits = true;
+ }
- if (send_32bits) {
- /* NEC transport, but modified protocol, used by at
-* least Apple and TiVo remotes */
- scancode = data->bits;
- IR_dprintk(1, "NEC (modified) scancode 0x%08x\n",
scancode);
- } else if ((address ^ not_address) != 0xff) {
- /* Extended NEC */
- scancode = address << 16 |
- not_address << 8 |
- command;
- IR_dprintk(1, "NEC (Ext) scancode 0x%06x\n", scancode);
- } else {
- /* Normal NEC */
- scancode = address << 8 | command;
- IR_dprintk(1, "NEC scancode 0x%04x\n", scancode);
- }
+ if (send_32bits) {
+ /* NEC transport, but modified protocol, used
+* by at least Apple and TiVo remotes */
+ scancode = data->bits;
+ IR_dprintk(1, "NEC (modified) scancode
0x%08x\n",
+ scancode);
+ } else if ((address ^ not_address) != 0xff) {
+ /* Extended NEC */
+ scancode = address << 16 |
+ not_address << 8 |
+ command;
+ IR_dprintk(1, "NEC (Ext) scancode 0x%06x\n",
+ scancode);
+ } else {
+ /* Normal NEC */
+ scancode = address << 8 | command;
+ IR_dprintk(1, "NEC scancode 0x%04x\n",
+ scancode);
+ }
- if (data->is_nec_x)
- data->necx_repeat = true;
+ if (data->is_nec_x)
+ data->necx_repeat = true;
- rc_keydown(dev, scancode, !dev->last_toggle);
+ rc_keydown(dev, scancode, !dev->last_toggle);
}
data->state = STATE_INACTIVE;
-- 1.8.5.2 (Apple Git-48)
--
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] drivers/media/rc/ir-nec-decode : add toggle feature
Hi, The IR NEC protocol decoder does not handle repeated key presses very well. It is regarded the same as a long key press, an thus triggers the auto-repeat functionality, which is not what I expected. The first patch solves the issue; the second patch fixes indentation inside the (new) if-block. I kept these 2 separate to make it more clear what the functional changes are, and which lines were only indented/reflown. Niels -- 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: soc_camera: pxa_camera device-tree support
On Sun, 15 Jun 2014, Robert Jarzmik wrote:
> Add device-tree support to pxa_camera host driver.
>
> Signed-off-by: Robert Jarzmik
> ---
> drivers/media/platform/soc_camera/pxa_camera.c | 80
> ++
> 1 file changed, 80 insertions(+)
>
> diff --git a/drivers/media/platform/soc_camera/pxa_camera.c
> b/drivers/media/platform/soc_camera/pxa_camera.c
> index d4df305..e48d821 100644
> --- a/drivers/media/platform/soc_camera/pxa_camera.c
> +++ b/drivers/media/platform/soc_camera/pxa_camera.c
> @@ -34,6 +34,7 @@
> #include
> #include
> #include
> +#include
>
> #include
>
> @@ -1650,9 +1651,74 @@ static struct soc_camera_host_ops
> pxa_soc_camera_host_ops = {
> .set_bus_param = pxa_camera_set_bus_param,
> };
>
> +static const struct of_device_id pxacamera_dt_ids[] = {
> + { .compatible = "mrvl,pxa_camera", },
as Documentation/devicetree/bindings/vendor-prefixes.txt defines, it
should be "marvell."
> + { }
> +};
> +
> +static int pxa_camera_pdata_from_dt(struct device *dev,
> + struct pxacamera_platform_data *pdata)
> +{
> + int err = 0;
> + struct device_node *np = dev->of_node;
> + struct v4l2_of_endpoint ep;
> +
> + dev_info(dev, "RJK: %s()\n", __func__);
I have nothing against attributing work to respective authors, but I don't
think this makes a lot of sense in the long run in the above form :) Once
you've verified that your binding is working and this function is working,
either remove this or make it more informative - maybe at the end of this
function, also listing a couple of important parameters, that you obtained
from DT.
> + err = of_property_read_u32(np, "mclk_10khz",
> +(u32 *)&pdata->mclk_10khz);
I think we'll be frowned upon for this :) PXA270 doesn't support CCF, does
it? Even if it doesn't we probably should use the standard
"clock-frequency" property in any case. Actually, I missed to mention on
this in my comments to your bindings documentation.
> + if (!err)
> + pdata->flags |= PXA_CAMERA_MCLK_EN;
> +
> + np = of_graph_get_next_endpoint(np, NULL);
> + if (!np) {
> + dev_err(dev, "could not find endpoint\n");
> + return -EINVAL;
> + }
> +
> + err = v4l2_of_parse_endpoint(np, &ep);
> + if (err) {
> + dev_err(dev, "could not parse endpoint\n");
> + return err;
> + }
> +
> + switch (ep.bus.parallel.bus_width) {
> + case 4:
> + pdata->flags |= PXA_CAMERA_DATAWIDTH_4;
> + break;
> + case 5:
> + pdata->flags |= PXA_CAMERA_DATAWIDTH_5;
> + break;
> + case 8:
> + pdata->flags |= PXA_CAMERA_DATAWIDTH_8;
> + break;
> + case 9:
> + pdata->flags |= PXA_CAMERA_DATAWIDTH_9;
> + break;
> + case 10:
> + pdata->flags |= PXA_CAMERA_DATAWIDTH_10;
> + break;
> + default:
> + break;
> + };
> +
> + if (ep.bus.parallel.flags & V4L2_MBUS_MASTER)
> + pdata->flags |= PXA_CAMERA_MASTER;
> + if (ep.bus.parallel.flags & V4L2_MBUS_HSYNC_ACTIVE_HIGH)
> + pdata->flags |= PXA_CAMERA_HSP;
> + if (ep.bus.parallel.flags & V4L2_MBUS_VSYNC_ACTIVE_HIGH)
> + pdata->flags |= PXA_CAMERA_VSP;
> + if (ep.bus.parallel.flags & V4L2_MBUS_PCLK_SAMPLE_RISING)
> + pdata->flags |= PXA_CAMERA_PCLK_EN | PXA_CAMERA_PCP;
> + if (ep.bus.parallel.flags & V4L2_MBUS_PCLK_SAMPLE_FALLING)
> + pdata->flags |= PXA_CAMERA_PCLK_EN;
> +
> + return 0;
> +}
> +
> static int pxa_camera_probe(struct platform_device *pdev)
> {
> struct pxa_camera_dev *pcdev;
> + struct pxacamera_platform_data pdata_dt;
> struct resource *res;
> void __iomem *base;
> int irq;
> @@ -1676,6 +1742,13 @@ static int pxa_camera_probe(struct platform_device
> *pdev)
> pcdev->res = res;
>
> pcdev->pdata = pdev->dev.platform_data;
> + if (&pdev->dev.of_node && !pcdev->pdata)
> + err = pxa_camera_pdata_from_dt(&pdev->dev, &pdata_dt);
> + if (err < 0)
> + return err;
> + else
> + pcdev->pdata = &pdata_dt;
This will Oops, if someone decides to dereference pcdev->pdata outside of
this function. pdata_dt is on stack and you store a pointer to it in your
device data... But since ->pdata doesn't seem to be used anywhere else in
this driver, maybe remove that struct member completely?
> +
> pcdev->platform_flags = pcdev->pdata->flags;
> if (!(pcdev->platform_flags & (PXA_CAMERA_DATAWIDTH_8 |
> PXA_CAMERA_DATAWIDTH_9 | PXA_CAMERA_DATAWIDTH_10))) {
> @@ -1799,10 +1872,17 @@ static const struct dev_pm_ops pxa_camera_pm = {
> .resume = pxa_camera_resume,
> };
>
> +static const struct of_device_id pxa_camera_of_match[] = {
> + { .compatible = "mrvl,pxa_camera", },
Ano
Re: [PATCH 1/2] media: soc_camera: pxa_camera documentation device-tree support
On Sun, 15 Jun 2014, Robert Jarzmik wrote:
> Add documentation for pxa_camera host interface.
As mentioned in another comment: this is driver DT bindings description,
not interface documentation.
Thanks
Guennadi
>
> Signed-off-by: Robert Jarzmik
> ---
> .../devicetree/bindings/media/pxa-camera.txt | 40
> ++
> 1 file changed, 40 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/media/pxa-camera.txt
>
> diff --git a/Documentation/devicetree/bindings/media/pxa-camera.txt
> b/Documentation/devicetree/bindings/media/pxa-camera.txt
> new file mode 100644
> index 000..568b63b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/pxa-camera.txt
> @@ -0,0 +1,40 @@
> +Marvell PXA camera host interface
> +
> +Required properties:
> + - compatible: Should be "mrvl,pxa_camera"
> + - reg: register base and size
> + - interrupts: the interrupt number
> + - any required generic properties defined in video-interfaces.txt
> +
> +Optional properties:
> + - mclk_10khz: host interface is driving MCLK, and MCLK rate is mclk_10khz *
> + 1 Hz.
> +
> +Example:
> +
> + pxa_camera: pxa_camera@5000 {
> + compatible = "mrvl,pxa_camera";
> + reg = <0x5000 0x1000>;
> + interrupts = <33>;
> +
> + clocks = <&pxa2xx_clks 24>;
> + clock-names = "camera";
> + status = "okay";
> +
> + mclk_10khz = <5000>;
> +
> + port {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + /* Parallel bus endpoint */
> + qci: endpoint@0 {
> + reg = <0>; /* Local endpoint # */
> + remote-endpoint = <&mt9m111_1>;
> + bus-width = <8>;/* Used data lines */
> + hsync-active = <0>; /* Active low */
> + vsync-active = <0>; /* Active low */
> + pclk-sample = <1>; /* Rising */
> + };
> + };
> + };
> --
> 2.0.0.rc2
>
--
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] Update sync-with-kernel to use installed kernel headers
Ping ? On Tuesday 03 June 2014 12:40:19 Laurent Pinchart wrote: > Kernel headers exported to userspace can contain kernel-specific > statements (such as __user annotations) that are removed when installing > the headers with 'make headers_install' in the kernel sources. Only > those headers must be used by userspace, raw headers are private to the > kernel. > > Update the sync-with-kernel make target to use the installed headers. > The user must install the kernel headers by running > > make headers_install > > in KERNEL_DIR prior to run sync-with-kernel. > > Signed-off-by: Laurent Pinchart > --- > Makefile.am | 45 +--- > contrib/freebsd/Makefile.am | 2 +- > contrib/freebsd/bsdify.sh | 2 +- > contrib/freebsd/patches/dvb-dmx-header.diff | 8 ++--- > contrib/freebsd/patches/dvb-osd-header.diff | 2 +- > contrib/freebsd/patches/dvb-video-header.diff | 8 ++--- > contrib/freebsd/patches/input-header.diff | 8 ++--- > contrib/freebsd/patches/ivtv-header.diff | 5 ++- > contrib/freebsd/patches/uinput-header.diff| 8 ++--- > contrib/freebsd/patches/videodev2-header.diff | 13 > lib/libdvbv5/Makefile.am | 2 +- > lib/libdvbv5/gen_dvb_structs.pl | 2 +- > utils/keytable/Makefile.am| 12 +++ > 13 files changed, 55 insertions(+), 62 deletions(-) > > diff --git a/Makefile.am b/Makefile.am > index 11baed1..35d0030 100644 > --- a/Makefile.am > +++ b/Makefile.am > @@ -12,31 +12,32 @@ EXTRA_DIST = include COPYING.libv4l README.libv4l > README.lib-multi-threading # custom targets > > sync-with-kernel: > - @if [ ! -f $(KERNEL_DIR)/include/uapi/linux/videodev2.h -o \ > - ! -f $(KERNEL_DIR)/include/uapi/linux/fb.h -o \ > - ! -f $(KERNEL_DIR)/include/uapi/linux/v4l2-controls.h -o \ > - ! -f $(KERNEL_DIR)/include/uapi/linux/v4l2-common.h -o \ > - ! -f $(KERNEL_DIR)/include/uapi/linux/v4l2-subdev.h -o \ > - ! -f $(KERNEL_DIR)/include/uapi/linux/v4l2-mediabus.h -o \ > - ! -f $(KERNEL_DIR)/include/uapi/linux/ivtv.h -o \ > - ! -f $(KERNEL_DIR)/include/uapi/linux/dvb/frontend.h -o \ > - ! -f $(KERNEL_DIR)/include/uapi/linux/dvb/dmx.h -o \ > - ! -f $(KERNEL_DIR)/include/uapi/linux/dvb/audio.h -o \ > - ! -f $(KERNEL_DIR)/include/uapi/linux/dvb/video.h ]; then \ > + @if [ ! -f $(KERNEL_DIR)/usr/include/linux/videodev2.h -o \ > + ! -f $(KERNEL_DIR)/usr/include/linux/fb.h -o \ > + ! -f $(KERNEL_DIR)/usr/include/linux/v4l2-controls.h -o \ > + ! -f $(KERNEL_DIR)/usr/include/linux/v4l2-common.h -o \ > + ! -f $(KERNEL_DIR)/usr/include/linux/v4l2-subdev.h -o \ > + ! -f $(KERNEL_DIR)/usr/include/linux/v4l2-mediabus.h -o \ > + ! -f $(KERNEL_DIR)/usr/include/linux/ivtv.h -o \ > + ! -f $(KERNEL_DIR)/usr/include/linux/dvb/frontend.h -o \ > + ! -f $(KERNEL_DIR)/usr/include/linux/dvb/dmx.h -o \ > + ! -f $(KERNEL_DIR)/usr/include/linux/dvb/audio.h -o \ > + ! -f $(KERNEL_DIR)/usr/include/linux/dvb/video.h ]; then \ > echo "Error you must set KERNEL_DIR to point to an extracted kernel > source dir"; \ +echo "and run 'make headers_install' in > \$$KERNEL_DIR."; > \ > exit 1; \ > fi > - cp -a $(KERNEL_DIR)/include/uapi/linux/videodev2.h > $(top_srcdir)/include/linux - cp -a $(KERNEL_DIR)/include/uapi/linux/fb.h > $(top_srcdir)/include/linux - cp -a > $(KERNEL_DIR)/include/uapi/linux/v4l2-controls.h > $(top_srcdir)/include/linux - cp -a > $(KERNEL_DIR)/include/uapi/linux/v4l2-common.h $(top_srcdir)/include/linux > - cp -a $(KERNEL_DIR)/include/uapi/linux/v4l2-subdev.h > $(top_srcdir)/include/linux - cp -a > $(KERNEL_DIR)/include/uapi/linux/v4l2-mediabus.h > $(top_srcdir)/include/linux - cp -a $(KERNEL_DIR)/include/uapi/linux/ivtv.h > $(top_srcdir)/include/linux - cp -a > $(KERNEL_DIR)/include/uapi/linux/dvb/frontend.h > $(top_srcdir)/include/linux/dvb - cp -a > $(KERNEL_DIR)/include/uapi/linux/dvb/dmx.h $(top_srcdir)/include/linux/dvb > - cp -a $(KERNEL_DIR)/include/uapi/linux/dvb/audio.h > $(top_srcdir)/include/linux/dvb - cp -a > $(KERNEL_DIR)/include/uapi/linux/dvb/video.h > $(top_srcdir)/include/linux/dvb + cp -a > $(KERNEL_DIR)/usr/include/linux/videodev2.h $(top_srcdir)/include/linux > + cp -a $(KERNEL_DIR)/usr/include/linux/fb.h $(top_srcdir)/include/linux > + cp -a $(KERNEL_DIR)/usr/include/linux/v4l2-controls.h > $(top_srcdir)/include/linux + cp -a > $(KERNEL_DIR)/usr/include/linux/v4l2-common.h $(top_srcdir)/include/linux > + cp -a $(KERNEL_DIR)/usr/include/linux/v4l2-subdev.h > $(top_srcdir)/include/linux + cp -a > $(KERNEL_DIR)/usr/include/linux/v4l2-mediabus.h $(top_srcdir)/include/linux > + cp -a $(KERNEL_DIR)/usr/include/linux/ivtv.h $(top_srcdir)/include/linux > +
Re: [PATCH 2/2] media: mt9m111: add device-tree documentation
Hi Robert,
On Sun, 15 Jun 2014, Robert Jarzmik wrote:
> Add documentation for the Micron mt9m111 image sensor.
A nitpick: this isn't documentation for the sensor:) This is driver DT
bindings' documentation.
Thanks
Guennadi
>
> Signed-off-by: Robert Jarzmik
> ---
> .../devicetree/bindings/media/i2c/mt9m111.txt | 28
> ++
> 1 file changed, 28 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/media/i2c/mt9m111.txt
>
> diff --git a/Documentation/devicetree/bindings/media/i2c/mt9m111.txt
> b/Documentation/devicetree/bindings/media/i2c/mt9m111.txt
> new file mode 100644
> index 000..ed5a334
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/i2c/mt9m111.txt
> @@ -0,0 +1,28 @@
> +Micron 1.3Mp CMOS Digital Image Sensor
> +
> +The Micron MT9M111 is a CMOS active pixel digital image sensor with an active
> +array size of 1280H x 1024V. It is programmable through a simple two-wire
> serial
> +interface.
> +
> +Required Properties:
> +- compatible: value should be "micron,mt9m111"
> +
> +For further reading on port node refer to
> +Documentation/devicetree/bindings/media/video-interfaces.txt.
> +
> +Example:
> +
> + i2c_master {
> + mt9m111@5d {
> + compatible = "micron,mt9m111";
> + reg = <0x5d>;
> +
> + remote = <&pxa_camera>;
> + port {
> + mt9m111_1: endpoint {
> + bus-width = <8>;
> + remote-endpoint = <&pxa_camera>;
> + };
> + };
> + };
> + };
> --
> 2.0.0.rc2
>
--
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: mt9m111: add device-tree suppport
Hi Robert,
Thanks for the patch.
On Sun, 15 Jun 2014, Robert Jarzmik wrote:
> Add device-tree support for mt9m111 camera sensor.
>
> Signed-off-by: Robert Jarzmik
> ---
> drivers/media/i2c/soc_camera/mt9m111.c | 21 +
> 1 file changed, 21 insertions(+)
>
> diff --git a/drivers/media/i2c/soc_camera/mt9m111.c
> b/drivers/media/i2c/soc_camera/mt9m111.c
> index ccf5940..7d283ea 100644
> --- a/drivers/media/i2c/soc_camera/mt9m111.c
> +++ b/drivers/media/i2c/soc_camera/mt9m111.c
> @@ -923,6 +923,12 @@ done:
> return ret;
> }
>
> +static int of_get_mt9m111_platform_data(struct device *dev,
> + struct soc_camera_subdev_desc *desc)
> +{
> + return 0;
> +}
Why do you need this function? I would just drop it.
> +
> static int mt9m111_probe(struct i2c_client *client,
>const struct i2c_device_id *did)
> {
> @@ -931,6 +937,15 @@ static int mt9m111_probe(struct i2c_client *client,
> struct soc_camera_subdev_desc *ssdd = soc_camera_i2c_to_desc(client);
> int ret;
>
> + if (client->dev.of_node) {
> + ssdd = devm_kzalloc(&client->dev, sizeof(*ssdd), GFP_KERNEL);
> + if (!ssdd)
> + return -ENOMEM;
> + client->dev.platform_data = ssdd;
> + ret = of_get_mt9m111_platform_data(&client->dev, ssdd);
> + if (ret < 0)
> + return ret;
> + }
> if (!ssdd) {
> dev_err(&client->dev, "mt9m111: driver needs platform data\n");
> return -EINVAL;
> @@ -1015,6 +1030,11 @@ static int mt9m111_remove(struct i2c_client *client)
>
> return 0;
> }
> +static const struct of_device_id mt9m111_of_match[] = {
> + { .compatible = "micron,mt9m111", },
Not a flaw in this patch, but someone might want to add "micron" to
Documentation/devicetree/bindings/vendor-prefixes.txt
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, mt9m111_of_match);
>
> static const struct i2c_device_id mt9m111_id[] = {
> { "mt9m111", 0 },
> @@ -1025,6 +1045,7 @@ MODULE_DEVICE_TABLE(i2c, mt9m111_id);
> static struct i2c_driver mt9m111_i2c_driver = {
> .driver = {
> .name = "mt9m111",
> + .of_match_table = of_match_ptr(mt9m111_of_match),
> },
> .probe = mt9m111_probe,
> .remove = mt9m111_remove,
> --
> 2.0.0.rc2
>
--
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 7/9] soc_camera: add support for dt binding soc_camera drivers
Hi Ben,
Thanks for an update.
On Sun, 15 Jun 2014, Ben Dooks wrote:
> Add initial support for OF based soc-camera devices that may be used
> by any of the soc-camera drivers. The driver itself will need converting
> to use OF.
>
> These changes allow the soc-camera driver to do the connecting of any
> async capable v4l2 device to the soc-camera driver. This has currently
> been tested on the Renesas Lager board.
>
> It currently only supports one input device per driver as this seems
> to be the standard connection for these devices.
You ignored most of my comments to the previous version of this your
patch. Please, revisit.
Thanks
Guennadi
> Signed-off-by: Ben Dooks
> ---
>
> Fixes since v1:
> - Fix i2c mclk name compatible with other drivers
> - Ensure of_node is put after use
> ---
> drivers/media/platform/soc_camera/soc_camera.c | 120
> -
> 1 file changed, 119 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/platform/soc_camera/soc_camera.c
> b/drivers/media/platform/soc_camera/soc_camera.c
> index 7fec8cd..eda67d7 100644
> --- a/drivers/media/platform/soc_camera/soc_camera.c
> +++ b/drivers/media/platform/soc_camera/soc_camera.c
> @@ -36,6 +36,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
>
> @@ -1581,6 +1582,121 @@ static void scan_async_host(struct soc_camera_host
> *ici)
> #define scan_async_host(ici) do {} while (0)
> #endif
>
> +#ifdef CONFIG_OF
> +static int soc_of_bind(struct soc_camera_host *ici,
> +struct device_node *ep,
> +struct device_node *remote)
> +{
> + struct soc_camera_device *icd;
> + struct soc_camera_desc sdesc = {.host_desc.bus_id = ici->nr,};
> + struct soc_camera_async_client *sasc;
> + struct soc_camera_async_subdev *sasd;
> + struct v4l2_async_subdev **asd_array;
> + struct i2c_client *client;
> + char clk_name[V4L2_SUBDEV_NAME_SIZE];
> + int ret;
> +
> + /* allocate a new subdev and add match info to it */
> + sasd = devm_kzalloc(ici->v4l2_dev.dev, sizeof(*sasd), GFP_KERNEL);
> + if (!sasd)
> + return -ENOMEM;
> +
> + asd_array = devm_kzalloc(ici->v4l2_dev.dev,
> + sizeof(struct v4l2_async_subdev **),
> + GFP_KERNEL);
> + if (!asd_array)
> + return -ENOMEM;
> +
> + sasd->asd.match.of.node = remote;
> + sasd->asd.match_type = V4L2_ASYNC_MATCH_OF;
> + asd_array[0] = &sasd->asd;
> +
> + /* Or shall this be managed by the soc-camera device? */
> + sasc = devm_kzalloc(ici->v4l2_dev.dev, sizeof(*sasc), GFP_KERNEL);
> + if (!sasc)
> + return -ENOMEM;
> +
> + /* HACK: just need a != NULL */
> + sdesc.host_desc.board_info = ERR_PTR(-ENODATA);
> +
> + ret = soc_camera_dyn_pdev(&sdesc, sasc);
> + if (ret < 0)
> + return ret;
> +
> + sasc->sensor = &sasd->asd;
> +
> + icd = soc_camera_add_pdev(sasc);
> + if (!icd) {
> + platform_device_put(sasc->pdev);
> + return -ENOMEM;
> + }
> +
> + sasc->notifier.subdevs = asd_array;
> + sasc->notifier.num_subdevs = 1;
> + sasc->notifier.bound = soc_camera_async_bound;
> + sasc->notifier.unbind = soc_camera_async_unbind;
> + sasc->notifier.complete = soc_camera_async_complete;
> +
> + icd->sasc = sasc;
> + icd->parent = ici->v4l2_dev.dev;
> +
> + client = of_find_i2c_device_by_node(remote);
> +
> + if (client)
> + snprintf(clk_name, sizeof(clk_name), "%d-%04x",
> + client->adapter->nr, client->addr);
> + else
> + snprintf(clk_name, sizeof(clk_name), "of-%s",
> + of_node_full_name(remote));
> +
> + icd->clk = v4l2_clk_register(&soc_camera_clk_ops, clk_name, "mclk",
> icd);
> + if (IS_ERR(icd->clk)) {
> + ret = PTR_ERR(icd->clk);
> + goto eclkreg;
> + }
> +
> + ret = v4l2_async_notifier_register(&ici->v4l2_dev, &sasc->notifier);
> + if (!ret)
> + return 0;
> +
> +eclkreg:
> + icd->clk = NULL;
> + platform_device_unregister(sasc->pdev);
> + dev_err(ici->v4l2_dev.dev, "group probe failed: %d\n", ret);
> +
> + return ret;
> +}
> +
> +static inline void scan_of_host(struct soc_camera_host *ici)
> +{
> + struct device_node *np = ici->v4l2_dev.dev->of_node;
> + struct device_node *epn = NULL;
> + struct device_node *ren;
> +
> + while (true) {
> + epn = of_graph_get_next_endpoint(np, epn);
> + if (!epn)
> + break;
> +
> + ren = of_graph_get_remote_port(epn);
> + if (!ren) {
> + pr_info("%s: no remote for %s\n",
> + __func__, of_node_full_name(epn));
> + continue;
> + }
> +
> + /* so we now have a remote node to connect */
