[ddbridge] suspend-to-disk takes about a minute ("I2C timeout") if vdr in use on ASUS P8H67-M EVO
Eric Lavarde wrote:
> On 12/12/11 03:29, Jonathan Nieder wrote:
>> Does suspend-to-disk ("echo
>> disk>/sys/power/state") work fine in general?
>
> No idea in general, but I made some tests.
Thanks for this, and sorry for the slow response.
> First, it kind of worked: after restarting the computer, I got error
> messages in dmesg like the following:
> [ 569.260555] PM: Image restored successfully.
> [ 569.260556] Restarting tasks ... done.
> [ 569.260643] PM: Basic memory bitmaps freed
> [ 569.260646] video LNXVIDEO:00: Restoring backlight state
> [ 569.336385] zd1211rw 1-1.4:1.0: firmware version 4725
> [ 569.376318] zd1211rw 1-1.4:1.0: zd1211b chip 0ace:1215 v4810 high 00-02-72
> AL2230_RF pa0 g--NS
> [ 569.394922] ADDRCONF(NETDEV_UP): wlan0: link is not ready
> [ 570.265915] I2C timeout
> [ 570.265921] IRS 0001
[...]
> [... hundreds of line of this type ...]
Ok, sounds like nothing good.
This error message comes from the ddb_i2c_cmd() function in the
ddbridge driver. I don't think it's supposed to fail like that. :)
Ralph, Oliver, any hints for debugging this? The above is with a
v3.1-based kernel. More details are below, at
, and in messages after the first one
at .
Thanks,
Jonathan
> [ 676.092300] tda18212: i2c_write error
> [ 677.089780] I2C timeout
> [ 677.090203] IRS 0001
> [ 677.090607] stv0367: i2c_write error
> [ 677.090737] c_release
> [ 677.091408] DDBridge :03:00.0: PCI INT A disabled
>
> And VDR was frozen for perhaps one minute, then it restarted itself, and
> then everything was fine. Only WLAN was still not working, unplug/plug the
> USB stick did make it work again.
>
> Then I modified my test script to set the wake-up time and disk > state but
> then my computer froze (see attached picture).
> After reboot, I tried again, shutting off nodm (for vdr-sxfe) and vdr before
> calling my test script. And the computer woke up!
--
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] em28xx: Add Terratec Cinergy HTC USB XS to em28xx-cards.c
This adds support for the Terratec Cinergy HTC USB XS which is similar to
the Terratec H5 by adding the USB-ids to the table. According to
http://linux.terratec.de it uses the same ICs and DVB-C works for me
using the firmware of the H5.
Signed-off-by: Holger Nelson
---
diff --git a/drivers/media/video/em28xx/em28xx-cards.c
b/drivers/media/video/em28xx/em28xx-cards.c
index 1704da0..a7cfded 100644
--- a/drivers/media/video/em28xx/em28xx-cards.c
+++ b/drivers/media/video/em28xx/em28xx-cards.c
@@ -1963,6 +1963,10 @@ struct usb_device_id em28xx_id_table[] = {
.driver_info = EM2882_BOARD_TERRATEC_HYBRID_XS },
{ USB_DEVICE(0x0ccd, 0x0043),
.driver_info = EM2870_BOARD_TERRATEC_XS },
+ { USB_DEVICE(0x0ccd, 0x008e), /* Cinergy HTC USB XS Rev. 1 */
+ .driver_info = EM2884_BOARD_TERRATEC_H5 },
+ { USB_DEVICE(0x0ccd, 0x00ac), /* Cinergy HTC USB XS Rev. 2 */
+ .driver_info = EM2884_BOARD_TERRATEC_H5 },
{ USB_DEVICE(0x0ccd, 0x10a2), /* H5 Rev. 1 */
.driver_info = EM2884_BOARD_TERRATEC_H5 },
{ USB_DEVICE(0x0ccd, 0x10ad), /* H5 Rev. 2 */
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC/PATCH] [media] dw2102: use symbolic names for dw2102_table indices
dw2102_properties et al refer to entries in the USB-id table using
hard-coded indices, as in "&dw2102_table[6]", which means adding new
entries before the end of the list has the potential to introduce bugs
in code elsewhere in the file.
Use C99-style initializers with symbolic names for each index to avoid
this. This way, other device tables wanting to reuse the USB ids can
use expressions like "&dw2102_table[TEVII_S630]" that do not change as
the entries in the table are reordered.
Signed-off-by: Jonathan Nieder
---
Patrick Boettcher wrote:
> Due to the use of the reference in the USB-id table adding the new set at
> the end of the list is actually the best way. Adding them in the middle will
> cause a lot of changes and bugs.
Good catch. That seems like an accident waiting to happen. How about
something like this (untested)?
drivers/media/dvb/dvb-usb/dw2102.c | 78 ++--
1 files changed, 48 insertions(+), 30 deletions(-)
diff --git a/drivers/media/dvb/dvb-usb/dw2102.c
b/drivers/media/dvb/dvb-usb/dw2102.c
index 64add7cb1fd3..9a1863dc7232 100644
--- a/drivers/media/dvb/dvb-usb/dw2102.c
+++ b/drivers/media/dvb/dvb-usb/dw2102.c
@@ -1435,22 +1435,40 @@ static int dw2102_rc_query(struct dvb_usb_device *d,
u32 *event, int *state)
return 0;
}
+enum dw2102_table_entry {
+ CYPRESS_DW2102,
+ CYPRESS_DW2101,
+ CYPRESS_DW2104,
+ TEVII_S650,
+ TERRATEC_CINERGY_S,
+ CYPRESS_DW3101,
+ TEVII_S630,
+ PROF_1100,
+ TEVII_S660,
+ PROF_7500,
+ GENIATECH_SU3000,
+ TERRATEC_CINERGY_S2,
+ TEVII_S480_1,
+ TEVII_S480_2,
+ X3M_SPC1400HD,
+};
+
static struct usb_device_id dw2102_table[] = {
- {USB_DEVICE(USB_VID_CYPRESS, USB_PID_DW2102)},
- {USB_DEVICE(USB_VID_CYPRESS, 0x2101)},
- {USB_DEVICE(USB_VID_CYPRESS, USB_PID_DW2104)},
- {USB_DEVICE(0x9022, USB_PID_TEVII_S650)},
- {USB_DEVICE(USB_VID_TERRATEC, USB_PID_CINERGY_S)},
- {USB_DEVICE(USB_VID_CYPRESS, USB_PID_DW3101)},
- {USB_DEVICE(0x9022, USB_PID_TEVII_S630)},
- {USB_DEVICE(0x3011, USB_PID_PROF_1100)},
- {USB_DEVICE(0x9022, USB_PID_TEVII_S660)},
- {USB_DEVICE(0x3034, 0x7500)},
- {USB_DEVICE(0x1f4d, 0x3000)},
- {USB_DEVICE(USB_VID_TERRATEC, 0x00a8)},
- {USB_DEVICE(0x9022, USB_PID_TEVII_S480_1)},
- {USB_DEVICE(0x9022, USB_PID_TEVII_S480_2)},
- {USB_DEVICE(0x1f4d, 0x3100)},
+ [CYPRESS_DW2102] = {USB_DEVICE(USB_VID_CYPRESS, USB_PID_DW2102)},
+ [CYPRESS_DW2101] = {USB_DEVICE(USB_VID_CYPRESS, 0x2101)},
+ [CYPRESS_DW2104] = {USB_DEVICE(USB_VID_CYPRESS, USB_PID_DW2104)},
+ [TEVII_S650] = {USB_DEVICE(0x9022, USB_PID_TEVII_S650)},
+ [TERRATEC_CINERGY_S] = {USB_DEVICE(USB_VID_TERRATEC,
USB_PID_CINERGY_S)},
+ [CYPRESS_DW3101] = {USB_DEVICE(USB_VID_CYPRESS, USB_PID_DW3101)},
+ [TEVII_S630] = {USB_DEVICE(0x9022, USB_PID_TEVII_S630)},
+ [PROF_1100] = {USB_DEVICE(0x3011, USB_PID_PROF_1100)},
+ [TEVII_S660] = {USB_DEVICE(0x9022, USB_PID_TEVII_S660)},
+ [PROF_7500] = {USB_DEVICE(0x3034, 0x7500)},
+ [GENIATECH_SU3000] = {USB_DEVICE(0x1f4d, 0x3000)},
+ [TERRATEC_CINERGY_S2] = {USB_DEVICE(USB_VID_TERRATEC, 0x00a8)},
+ [TEVII_S480_1] = {USB_DEVICE(0x9022, USB_PID_TEVII_S480_1)},
+ [TEVII_S480_2] = {USB_DEVICE(0x9022, USB_PID_TEVII_S480_2)},
+ [X3M_SPC1400HD] = {USB_DEVICE(0x1f4d, 0x3100)},
{ }
};
@@ -1610,15 +1628,15 @@ static struct dvb_usb_device_properties
dw2102_properties = {
.num_device_descs = 3,
.devices = {
{"DVBWorld DVB-S 2102 USB2.0",
- {&dw2102_table[0], NULL},
+ {&dw2102_table[CYPRESS_DW2102], NULL},
{NULL},
},
{"DVBWorld DVB-S 2101 USB2.0",
- {&dw2102_table[1], NULL},
+ {&dw2102_table[CYPRESS_DW2101], NULL},
{NULL},
},
{"TerraTec Cinergy S USB",
- {&dw2102_table[4], NULL},
+ {&dw2102_table[TERRATEC_CINERGY_S], NULL},
{NULL},
},
}
@@ -1664,11 +1682,11 @@ static struct dvb_usb_device_properties
dw2104_properties = {
.num_device_descs = 2,
.devices = {
{ "DVBWorld DW2104 USB2.0",
- {&dw2102_table[2], NULL},
+ {&dw2102_table[CYPRESS_DW2104], NULL},
{NULL},
},
{ "TeVii S650 USB2.0",
- {&dw2102_table[3], NULL},
+ {&dw2102_table[TEVII_S650], NULL},
{NULL},
},
}
@@ -1715,7 +1733,7 @@ static struct dvb_usb_device_properties dw3101_properties
= {
.num_device_descs = 1,
.devices = {
{ "DVB
Re: [RFCv1] add DTMB support for DVB API
On 12/23/2011 03:38 PM, Andreas Oberritter wrote:
On 22.12.2011 22:30, Antti Palosaari wrote:
@@ -201,6 +205,9 @@ typedef enum fe_guard_interval {
GUARD_INTERVAL_1_128,
GUARD_INTERVAL_19_128,
GUARD_INTERVAL_19_256,
+GUARD_INTERVAL_PN420,
+GUARD_INTERVAL_PN595,
+GUARD_INTERVAL_PN945,
} fe_guard_interval_t;
What does PN mean in this context?
It stays for pseudo noise.
Generally those are rather same as PN420 = GI 1/4, PN595 = GI 1/6, PN945
= GI 1/9. But as a old DVB-T GI, cyclic prefix, is rather likely some
static data without any real payload meaning, the PN GI is enchanted
inserting some data that have meaning. Also PN GI is boosted, like
doubled TX power in order to easy sync.
Here is the one good research paper which compares DVB-T and DTMB [1]
And this Wikipedia page is rather helful too [2]
As I have read much more docs today I have now some changes I want to do:
CARRIER
===
typedef enum fe_transmit_mode {
TRANSMISSION_MODE_C=1,
TRANSMISSION_MODE_C=3780,
} fe_transmit_mode_t;
(If not "=" mark is not possible then use C1, C3780)
Instead of adding new carrier param, just use exiting one since it seems
to be just suitable. Extend DTMB modes C=1 and C=3780.
GUARD INTERVAL (PN-mode)
typedef enum fe_guard_interval {
GUARD_INTERVAL_PN420,
GUARD_INTERVAL_PN595,
GUARD_INTERVAL_PN945,
} fe_guard_interval_t;
CODE RATE
=
typedef enum fe_code_rate {
FEC_04,
FEC_06,
FEC_08,
} fe_code_rate_t;
I have strong feeling old FECs, 1 over 2, are 100% suitable only for the
Reed-Solomon coding. For BCH + LDPC coding those are not so suitable as
code rate is not exact and thus with LDPC 0.4/0.6/0.8 is used. I have
mentioned that earlier too, but after I read that [1] I think it is just
like that. It is not big issue but still I would like to use those
instead old.
Otherwise FEC_2_5 for code rate 0.4 should be defined.
MODULATION (constellation)
==
typedef enum fe_modulation {
QAM_4_NR,
} fe_modulation_t;
I feel QAM4-NR fits here too, since it is mentioned every document just
one more supported modulation like QAM32, QAM16, QAM4...
INTERLEAVING
typedef enum fe_interleaving {
INTERLEAVING_240,
INTERLEAVING_720,
} fe_interleaving_t;
I think better to add enum for all possible interleavers we can have. I
suspect there will be very limited amount of interleave settings
supported by each DTV modulation, thus enum is good choice.
That's all. I will wait comments maybe tomorrow night and make new
propose or RFC. I hope comments. And all those comments given are taken
accepted unless I replied something back. And please look research paper
[1], it is very good. Happy Xmas!
[1] Analysis and Performance Comparison of DVB-T and DTMB Systems for
Terrestrial Digital TV
http://hal.archives-ouvertes.fr/docs/00/32/58/24/PDF/MLIU_ICCS2008.pdf
[2] OFDM system comparison table
http://en.wikipedia.org/wiki/OFDM#OFDM_system_comparison_table
regards
Antti
--
http://palosaari.fi/
--
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: [RFCv1] add DTMB support for DVB API
On 23.12.2011 22:54, Antti Palosaari wrote:
> On 12/23/2011 12:55 PM, Mauro Carvalho Chehab wrote:
>> On 22-12-2011 19:30, Antti Palosaari wrote:
>>> Rename DMB-TH to DTMB.
>>
>> Patrick seems to believe that CTTB is a better name. In any case,
>> whatever name we pick, I think that the DocBook specs (and
>> maybe a comment at the header file) should point all the known
>> ways to call this standard. So, I'm fine with any choice.
>
> I am not going to change it for CTTB unless there is no some document
> given says it is more correct than DTMB. I have looked very many papers
> over there and DTMB is absolutely more common as far as I gave seen.
>
>>> Add few new values for existing parameters.
>>>
>>> Add two new parameters, interleaving and carrier.
>>> DTMB supports interleavers: 240 and 720.
>>> DTMB supports carriers: 1 and 3780.
>>
>> The API change looks sane to my eyes. I have just a couple
>> comments below.
>
> I think I will add carrier modes to enum fe_transmit_mode... I will send
> new propose soon.
>
>>> @@ -169,8 +170,11 @@ typedef enum fe_modulation {
>>> APSK_16,
>>> APSK_32,
>>> DQPSK,
>>> +QAM_4_NR,
>>
>> While the NR is generally associated with the modulation,
>> this is a channel code, and not part of the modulation itself
>> (btw, the DVBv3 API calls it as "constellation").
>>
>> Ok, it is easier to add it here, but maybe it would be safer
>> to add a separate field for channel coding that would be used
>> to enable or disable the Nordstrom-Robinson code.
>>
>> This is currently used only with QAM-4, but nothing prevents this
>> parity code to be added to other types of modulation.
>>
>>> } fe_modulation_t;
>>>
>>> +#define QAM_4 QPSK
>>
>> IMHO, this is overkill, but I'm ok with that.
>
> Anyone else have comment about that?
I don't think we should create such defines unless needed for backwards
compatibility, which is not the case.
Regards,
Andreas
--
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: [RFCv1] add DTMB support for DVB API
On 12/23/2011 12:55 PM, Mauro Carvalho Chehab wrote:
On 22-12-2011 19:30, Antti Palosaari wrote:
Rename DMB-TH to DTMB.
Patrick seems to believe that CTTB is a better name. In any case,
whatever name we pick, I think that the DocBook specs (and
maybe a comment at the header file) should point all the known
ways to call this standard. So, I'm fine with any choice.
I am not going to change it for CTTB unless there is no some document
given says it is more correct than DTMB. I have looked very many papers
over there and DTMB is absolutely more common as far as I gave seen.
Add few new values for existing parameters.
Add two new parameters, interleaving and carrier.
DTMB supports interleavers: 240 and 720.
DTMB supports carriers: 1 and 3780.
The API change looks sane to my eyes. I have just a couple
comments below.
I think I will add carrier modes to enum fe_transmit_mode... I will send
new propose soon.
@@ -169,8 +170,11 @@ typedef enum fe_modulation {
APSK_16,
APSK_32,
DQPSK,
+QAM_4_NR,
While the NR is generally associated with the modulation,
this is a channel code, and not part of the modulation itself
(btw, the DVBv3 API calls it as "constellation").
Ok, it is easier to add it here, but maybe it would be safer
to add a separate field for channel coding that would be used
to enable or disable the Nordstrom-Robinson code.
This is currently used only with QAM-4, but nothing prevents this
parity code to be added to other types of modulation.
} fe_modulation_t;
+#define QAM_4 QPSK
IMHO, this is overkill, but I'm ok with that.
Anyone else have comment about that?
regards
Antti
--
http://palosaari.fi/
--
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
Multiple channel capture support in V4l2 layer
Hi, I am trying to implement v4l2 slave driver for ML86V76654 digital video decoder that converts NTSC, PAL, SECAM analog video signals into the YCbCr standard digital format. This codec takes 4 analog inputs(2 analog camera + 2 ext video in) and encodes in to digital(only one at a time). The driver should be able to switch between capture channels upon request from user space application. I couldn't find the support for multiple capture channels on a single device in v4l2 layer. Please correct me if I am wrong. Ideally I want the v4l2 slave driver to have following feature. 1. ioctl that can be used to enumerate/get/set the capture channels on the video encoder. 2. Able to capture video from the currently set capture channel and pass to higher layers. Which is the best way to implement this support? Regards, Dilip -- 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
cron job: media_tree daily build: ERRORS
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 Dec 23 19:00:18 CET 2011 git hash:875e2e3edf48a206c64195666cf408dd3d119137 gcc version: i686-linux-gcc (GCC) 4.6.1 host hardware:x86_64 host os: 3.1-2.slh.1-amd64 linux-git-arm-eabi-enoxys: ERRORS linux-git-arm-eabi-omap: ERRORS linux-git-armv5-ixp: WARNINGS linux-git-i686: WARNINGS linux-git-m32r: OK linux-git-mips: WARNINGS linux-git-powerpc64: WARNINGS linux-git-x86_64: WARNINGS linux-2.6.31.12-i686: ERRORS linux-2.6.32.6-i686: WARNINGS linux-2.6.33-i686: WARNINGS linux-2.6.34-i686: WARNINGS linux-2.6.35.3-i686: WARNINGS linux-2.6.36-i686: WARNINGS linux-2.6.37-i686: WARNINGS linux-2.6.38.2-i686: WARNINGS linux-2.6.39.1-i686: WARNINGS linux-3.0-i686: WARNINGS linux-3.1-i686: WARNINGS linux-3.2-rc1-i686: WARNINGS linux-2.6.31.12-x86_64: ERRORS linux-2.6.32.6-x86_64: WARNINGS linux-2.6.33-x86_64: WARNINGS linux-2.6.34-x86_64: WARNINGS linux-2.6.35.3-x86_64: WARNINGS linux-2.6.36-x86_64: WARNINGS linux-2.6.37-x86_64: WARNINGS linux-2.6.38.2-x86_64: WARNINGS linux-2.6.39.1-x86_64: WARNINGS linux-3.0-x86_64: WARNINGS linux-3.1-x86_64: WARNINGS linux-3.2-rc1-x86_64: WARNINGS spec-git: WARNINGS sparse: ERRORS 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 V4L-DVB specification 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
[GIT PULL for v3.2-rc] media fix
Linus, Please pull from: git://git.kernel.org/pub/scm/linux/kernel/git/mchehab/linux-media v4l_for_linus For a last time fix for the omap3 driver. Thanks and Happy Seasons! Mauro Latest commit at the branch: c070e38e4ee005f55895df177a9e14d90d6204b3 [media] omap3isp: Fix crash caused by subdevs now having a pointer to devnodes The following changes since commit 4b5d8da88e3fab76700e89488a8c65c54facb9a3: Revert "[media] af9015: limit I2C access to keep FW happy" (2011-12-12 16:02:15 -0200) are available in the git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/mchehab/linux-media v4l_for_linus for you to fetch changes up to c070e38e4ee005f55895df177a9e14d90d6204b3: [media] omap3isp: Fix crash caused by subdevs now having a pointer to devnodes (2011-12-19 18:07:41 -0200) Laurent Pinchart (1): [media] omap3isp: Fix crash caused by subdevs now having a pointer to devnodes drivers/media/video/omap3isp/ispccdc.c |2 +- drivers/media/video/omap3isp/ispstat.c |2 +- 2 files changed, 2 insertions(+), 2 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 9/9] drivers/staging/media/as102/as102_usb_drv.c: shift position of allocation code
The conditional after the kzalloc says that the tested expression should
never be true, but if it were, the allocated data would have to be freed.
This change just moves the allocation below the test, to avoid any
possibility of the problem.
A simplified version of the semantic match that finds the problem is as
follows: (http://coccinelle.lip6.fr)
//
@r exists@
local idexpression x;
statement S;
identifier f1;
position p1,p2;
expression *ptr != NULL;
@@
x@p1 = \(kmalloc\|kzalloc\|kcalloc\)(...);
...
if (x == NULL) S
<... when != x
when != if (...) { <+...x...+> }
x->f1
...>
(
return \(0\|<+...x...+>\|ptr\);
|
return@p2 ...;
)
@script:python@
p1 << r.p1;
p2 << r.p2;
@@
print "* file: %s kmalloc %s return %s" % (p1[0].file,p1[0].line,p2[0].line)
//
Signed-off-by: Julia Lawall
---
drivers/staging/media/as102/as102_usb_drv.c | 12 ++--
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/staging/media/as102/as102_usb_drv.c
b/drivers/staging/media/as102/as102_usb_drv.c
index 7bcb28c..d775be0 100644
--- a/drivers/staging/media/as102/as102_usb_drv.c
+++ b/drivers/staging/media/as102/as102_usb_drv.c
@@ -353,12 +353,6 @@ static int as102_usb_probe(struct usb_interface *intf,
ENTER();
- as102_dev = kzalloc(sizeof(struct as102_dev_t), GFP_KERNEL);
- if (as102_dev == NULL) {
- err("%s: kzalloc failed", __func__);
- return -ENOMEM;
- }
-
/* This should never actually happen */
if ((sizeof(as102_usb_id_table) / sizeof(struct usb_device_id)) !=
(sizeof(as102_device_names) / sizeof(const char *))) {
@@ -366,6 +360,12 @@ static int as102_usb_probe(struct usb_interface *intf,
return -EINVAL;
}
+ as102_dev = kzalloc(sizeof(struct as102_dev_t), GFP_KERNEL);
+ if (as102_dev == NULL) {
+ err("%s: kzalloc failed", __func__);
+ return -ENOMEM;
+ }
+
/* Assign the user-friendly device name */
for (i = 0; i < (sizeof(as102_usb_id_table) /
sizeof(struct usb_device_id)); i++) {
--
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 7/9] drivers/media/video/davinci/vpbe.c: introduce missing kfree
vpbe_dev needs to be freed before leaving the function in an error case.
A simplified version of the semantic match that finds the problem is as
follows: (http://coccinelle.lip6.fr)
//
@r exists@
local idexpression x;
statement S;
identifier f1;
position p1,p2;
expression *ptr != NULL;
@@
x@p1 = \(kmalloc\|kzalloc\|kcalloc\)(...);
...
if (x == NULL) S
<... when != x
when != if (...) { <+...x...+> }
x->f1
...>
(
return \(0\|<+...x...+>\|ptr\);
|
return@p2 ...;
)
@script:python@
p1 << r.p1;
p2 << r.p2;
@@
print "* file: %s kmalloc %s return %s" % (p1[0].file,p1[0].line,p2[0].line)
//
Signed-off-by: Julia Lawall
---
drivers/media/video/davinci/vpbe.c |4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/media/video/davinci/vpbe.c
b/drivers/media/video/davinci/vpbe.c
index d773d30..99658ba 100644
--- a/drivers/media/video/davinci/vpbe.c
+++ b/drivers/media/video/davinci/vpbe.c
@@ -811,8 +811,10 @@ static __devinit int vpbe_probe(struct platform_device
*pdev)
if (cfg->outputs->num_modes > 0)
vpbe_dev->current_timings = vpbe_dev->cfg->outputs[0].modes[0];
- else
+ else {
+ kfree(vpbe_dev);
return -ENODEV;
+ }
/* set the driver data in platform device */
platform_set_drvdata(pdev, vpbe_dev);
--
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: [Linaro-mm-sig] [RFC v3 0/2] Introduce DMA buffer sharing mechanism
On Fri, Dec 23, 2011 at 4:08 AM, Semwal, Sumit wrote: > On Wed, Dec 21, 2011 at 1:50 AM, Dave Airlie wrote: > >>> >>> I think this is a really good v1 version of dma_buf. It contains all the >>> required bits (with well-specified semantics in the doc patch) to >>> implement some basic use-cases and start fleshing out the integration with >>> various subsystem (like drm and v4l). All the things still under >>> discussion like >>> - userspace mmap support >>> - more advanced (and more strictly specified) coherency models >>> - and shared infrastructure for implementing exporters >>> are imo much clearer once we have a few example drivers at hand and a >>> better understanding of some of the insane corner cases we need to be able >>> to handle. >>> >>> And I think any risk that the resulting clarifications will break a basic >>> use-case is really minimal, so I think it'd be great if this could go into >>> 3.3 (maybe as some kind of staging/experimental infrastructure). >>> >>> Hence for both patches: >>> Reviewed-by: Daniel Vetter >> >> Yeah I'm with Daniel, I like this one, I can definitely build the drm >> buffer sharing layer on top of this. >> >> How do we see this getting merged? I'm quite happy to push it to Linus >> if we don't have an identified path, though it could go via a Linaro >> tree as well. >> >> so feel free to add: >> Reviewed-by: Dave Airlie > Thanks Daniel and Dave! > > I guess we can start with staging for 3.3, and see how it shapes up. I > will post the latest patch version pretty soon. not sure about staging, but could make sense to mark as experimental. > Arnd, Dave: do you have any preference on the path it takes to get > merged? In my mind, Linaro tree might make more sense, but I would > leave it upto you gentlemen. Looks like Dave is making some progress on drm usage of buffer sharing between gpu's.. if that is ready to go in at the same time, it might be a bit logistically simpler for him to put dmabuf in the same pull req. I don't have strong preference one way or another, so do what is collectively simpler ;-) BR, -R >> >> Dave. > Best regards, > ~Sumit. > -- > 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 -- 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: [Linaro-mm-sig] [RFC v2 1/2] dma-buf: Introduce dma buffer sharing mechanism
On Fri, Dec 23, 2011 at 4:00 AM, Semwal, Sumit wrote: > On Wed, Dec 21, 2011 at 10:57 PM, Arnd Bergmann wrote: >> On Tuesday 20 December 2011, Daniel Vetter wrote: >>> > I'm thinking for a first version, we can get enough mileage out of it by >>> > saying: >>> > 1) only exporter can mmap to userspace >>> > 2) only importers that do not need CPU access to buffer.. > > Thanks Rob - and the exporter can do the mmap outside of dma-buf > usage, right? Yes > I mean, we don't need to provide an mmap to dma_buf() > and restrict it to exporter, when the exporter has more 'control' of > the buffer anyways. No, if it is only for the exporter, it really doesn't need to be in dmabuf (ie. the exporter already knows how he is) BR, -R >> > BR, > ~Sumit. -- 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: MEM2MEM devices: how to handle sequence number?
Hi Laurent, On 12/23/2011 12:54 PM, Laurent Pinchart wrote: > diff --git a/drivers/media/video/videobuf2-core.c > b/drivers/media/video/videobuf2-core.c > index 1250662..7d8a88b 100644 > --- a/drivers/media/video/videobuf2-core.c > +++ b/drivers/media/video/videobuf2-core.c > @@ -1127,6 +1127,7 @@ int vb2_qbuf(struct vb2_queue *q, struct > v4l2_buffer *b) > > */ > > list_add_tail(&vb->queued_entry,&q->queued_list); > vb->state = VB2_BUF_STATE_QUEUED; > > + vb->v4l2_buf.sequence = b->sequence; > > /* > > * If already streaming, give the buffer to driver for > processing. Right, such patch is definitely needed. Please resend it with 'signed-off-by' annotation. >>> >>> I'm not too sure about that. Isn't the sequence number supposed to be >>> ignored by drivers on video output devices ? The documentation is a bit >>> terse on the subject, all it says is >>> >>> __u32 sequence Set by the driver, counting the frames in the >>> sequence. >> >> We can also update the documentation if needed. IMHO copying sequence >> number in mem2mem case if there is 1:1 relation between the buffers is a >> good idea. > > My point is that sequence numbers are currently not applicable to video output > devices, at least according to the documentation. Applications will just set > them to 0. Looks like the documentation wasn't updated when the Memory-To-Memory interface has been introduced. > I think it would be better to have the m2m driver set the sequence number > internally on the video output node by incrementing an counter, and pass it > down the pipeline to the video capture node. It sounds reasonable. Currently the sequence is zeroed at streamon in the capture drivers. Similar behaviour could be assured by m2m drivers. In Javier's case it's probably more reliable to check the sequence numbers contiguity directly at the image source driver's device node. Although when m2m driver sets the sequence number internally on a video output queue it could make sense to have the buffer's sequence number updated upon return from VIDIOC_QBUF. What do you think ? This would be needed for the object detection interface if we wanted to associate object detection result with a frame sequence number. As far as the implementation is concerned, m2m and output drivers (with selected capabilities only?) would have to update buffer sequence number from within buf_queue vb2 queue op. -- Regards, Sylwester -- 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: [RFCv1] add DTMB support for DVB API
On 22.12.2011 22:30, Antti Palosaari wrote:
> @@ -201,6 +205,9 @@ typedef enum fe_guard_interval {
> GUARD_INTERVAL_1_128,
> GUARD_INTERVAL_19_128,
> GUARD_INTERVAL_19_256,
> +GUARD_INTERVAL_PN420,
> +GUARD_INTERVAL_PN595,
> +GUARD_INTERVAL_PN945,
> } fe_guard_interval_t;
What does PN mean in this context?
Regards,
Andreas
--
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
[RFC] remove dtv_property_dump et al
On 22.12.2011 22:30, Antti Palosaari wrote:
> Rename DMB-TH to DTMB.
>
> Add few new values for existing parameters.
>
> Add two new parameters, interleaving and carrier.
> DTMB supports interleavers: 240 and 720.
> DTMB supports carriers: 1 and 3780.
>
> Signed-off-by: Antti Palosaari
> ---
> drivers/media/dvb/dvb-core/dvb_frontend.c | 19 ++-
> drivers/media/dvb/dvb-core/dvb_frontend.h |3 +++
> include/linux/dvb/frontend.h | 13 +++--
> include/linux/dvb/version.h |2 +-
> 4 files changed, 33 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/media/dvb/dvb-core/dvb_frontend.c
> b/drivers/media/dvb/dvb-core/dvb_frontend.c
> index 821b225..ec2cbae 100644
> --- a/drivers/media/dvb/dvb-core/dvb_frontend.c
> +++ b/drivers/media/dvb/dvb-core/dvb_frontend.c
> @@ -924,6 +924,8 @@ static struct dtv_cmds_h dtv_cmds[DTV_MAX_COMMAND +
> 1] = {
> _DTV_CMD(DTV_CODE_RATE_LP, 1, 0),
> _DTV_CMD(DTV_GUARD_INTERVAL, 1, 0),
> _DTV_CMD(DTV_TRANSMISSION_MODE, 1, 0),
> +_DTV_CMD(DTV_CARRIER, 1, 0),
> +_DTV_CMD(DTV_INTERLEAVING, 1, 0),
>
> _DTV_CMD(DTV_ISDBT_PARTIAL_RECEPTION, 1, 0),
> _DTV_CMD(DTV_ISDBT_SOUND_BROADCASTING, 1, 0),
> @@ -974,6 +976,8 @@ static struct dtv_cmds_h dtv_cmds[DTV_MAX_COMMAND +
> 1] = {
> _DTV_CMD(DTV_GUARD_INTERVAL, 0, 0),
> _DTV_CMD(DTV_TRANSMISSION_MODE, 0, 0),
> _DTV_CMD(DTV_HIERARCHY, 0, 0),
> +_DTV_CMD(DTV_CARRIER, 0, 0),
> +_DTV_CMD(DTV_INTERLEAVING, 0, 0),
>
> _DTV_CMD(DTV_ENUM_DELSYS, 0, 0),
> };
Adding this twice is wrong.
Almost every time new commands were added in the recent past, either the
command was not added to this table at all or the command was added more
than once. How about removing this array instead? It's only purpose is
to output ioctl parameters, which can be done with strace as well.
Actually, I have two patches queued doing that (see attachments). They
are based on 3.0.
Regards,
Andreas
>From 0f9294ce272a853bb0f4513bbd07048938c78db2 Mon Sep 17 00:00:00 2001
From: Andreas Oberritter
Date: Tue, 13 Sep 2011 11:48:14 +
Subject: [PATCH 1/2] [RFC] DVB: remove dtv_property_dump()
- dtv_property_dump is the only user of struct dtv_cmds_h,
which mistakenly got exported to userspace.
- It is used to print ioctl parameters, if debugging is enabled.
The array used to print parameters contains both unused ("set")
and invalid entries, i.e. DTV_CODE_RATE_HP and others are
defined twice.
- When a new DTV command gets added, it's a common mistake to
forget to add it to this array.
- It's possible to use strace instead or to write a simple
LD_PRELOADable library.
Signed-off-by: Andreas Oberritter
---
drivers/media/dvb/dvb-core/dvb_frontend.c | 116 -
include/linux/dvb/frontend.h | 11 ---
2 files changed, 0 insertions(+), 127 deletions(-)
diff --git a/drivers/media/dvb/dvb-core/dvb_frontend.c b/drivers/media/dvb/dvb-core/dvb_frontend.c
index 521b695..8274a6d 100644
--- a/drivers/media/dvb/dvb-core/dvb_frontend.c
+++ b/drivers/media/dvb/dvb-core/dvb_frontend.c
@@ -895,119 +895,6 @@ static int dvb_frontend_clear_cache(struct dvb_frontend *fe)
return 0;
}
-#define _DTV_CMD(n, s, b) \
-[n] = { \
- .name = #n, \
- .cmd = n, \
- .set = s,\
- .buffer = b \
-}
-
-static struct dtv_cmds_h dtv_cmds[DTV_MAX_COMMAND + 1] = {
- _DTV_CMD(DTV_TUNE, 1, 0),
- _DTV_CMD(DTV_CLEAR, 1, 0),
-
- /* Set */
- _DTV_CMD(DTV_FREQUENCY, 1, 0),
- _DTV_CMD(DTV_BANDWIDTH_HZ, 1, 0),
- _DTV_CMD(DTV_MODULATION, 1, 0),
- _DTV_CMD(DTV_INVERSION, 1, 0),
- _DTV_CMD(DTV_DISEQC_MASTER, 1, 1),
- _DTV_CMD(DTV_SYMBOL_RATE, 1, 0),
- _DTV_CMD(DTV_INNER_FEC, 1, 0),
- _DTV_CMD(DTV_VOLTAGE, 1, 0),
- _DTV_CMD(DTV_TONE, 1, 0),
- _DTV_CMD(DTV_PILOT, 1, 0),
- _DTV_CMD(DTV_ROLLOFF, 1, 0),
- _DTV_CMD(DTV_DELIVERY_SYSTEM, 1, 0),
- _DTV_CMD(DTV_HIERARCHY, 1, 0),
- _DTV_CMD(DTV_CODE_RATE_HP, 1, 0),
- _DTV_CMD(DTV_CODE_RATE_LP, 1, 0),
- _DTV_CMD(DTV_GUARD_INTERVAL, 1, 0),
- _DTV_CMD(DTV_TRANSMISSION_MODE, 1, 0),
-
- _DTV_CMD(DTV_ISDBT_PARTIAL_RECEPTION, 1, 0),
- _DTV_CMD(DTV_ISDBT_SOUND_BROADCASTING, 1, 0),
- _DTV_CMD(DTV_ISDBT_SB_SUBCHANNEL_ID, 1, 0),
- _DTV_CMD(DTV_ISDBT_SB_SEGMENT_IDX, 1, 0),
- _DTV_CMD(DTV_ISDBT_SB_SEGMENT_COUNT, 1, 0),
- _DTV_CMD(DTV_ISDBT_LAYER_ENABLED, 1, 0),
- _DTV_CMD(DTV_ISDBT_LAYERA_FEC, 1, 0),
- _DTV_CMD(DTV_ISDBT_LAYERA_MODULATION, 1, 0),
- _DTV_CMD(DTV_ISDBT_LAYERA_SEGMENT_COUNT, 1, 0),
- _DTV_CMD(DTV_ISDBT_LAYERA_TIME_INTERLEAVING, 1, 0),
- _DTV_CMD(DTV_ISDBT_LAYERB_FEC, 1, 0),
- _DTV_CMD(DTV_ISDBT_LAYERB_MODULATION, 1, 0),
- _DTV_CMD(DTV_ISDBT_LAYERB_SEGMENT_COUNT, 1, 0),
- _DTV_CMD(DTV_ISDBT_LAYERB_TIME_INTERLEAVING, 1, 0),
- _DTV_CMD(DTV_ISDBT_LAYERC_FEC, 1, 0),
- _DTV_CMD(DTV_ISDBT_LAYERC_MODULATION, 1, 0),
- _DTV_CMD(DTV_ISDBT_LAYERC_SEGMENT_COUNT, 1, 0),
- _DTV_CMD(DTV_ISDBT_LAYERC_TIME_INTERLEAVING, 1, 0),
-
- _DTV_CMD(DTV_ISDBT_PARTIAL_RECEPTION, 0, 0),
- _DTV_CMD(DTV_ISDBT_SOUND_BROADCASTING, 0,
Re: [RFC PATCH v2 4/8] media: videobuf2: introduce VIDEOBUF2_PAGE memops
Hi, On Fri, Dec 23, 2011 at 6:38 PM, Marek Szyprowski wrote: > Hello, > > On Friday, December 23, 2011 10:51 AM Ming Lei wrote: > >> On Fri, Dec 23, 2011 at 5:34 PM, Marek Szyprowski >> wrote: >> >> >> For example, on ARM, there is very limited kernel virtual address space >> >> reserved >> >> for DMA coherent buffer mapping, the default size is about 2M if I >> >> don't remember mistakenly. >> > >> > It can be easily increased for particular boards, there is no problem with >> > this. >> >> It is not easily to increase it because there is very limited space reserved >> for >> this purpose, see Documentation/arm/memory.txt. Also looks like it is >> not configurable. > > It is really not a big issue to increase it by a few MBytes. IMO, the extra few MBytes are not helpful for the issue at all, considered the kernel virtual memory address fragmentation problem, you know there is only several MBytes DMA coherent virtual address space on ARM. > >> >> > I understand that there might be some speed issues with coherent >> >> > (uncached) >> >> > userspace mappings, but I would solve it in completely different way. >> >> > The interface >> >> >> >> Also there is poor performance inside kernel space, see [1] >> > >> > Your driver doesn't access video data inside kernel space, so this is also >> > not an issue. >> >> Why not introduce it so that other drivers(include face detection) can >> benefit with it? :-) > > We can get back into this once a driver which really benefits from comes. In fact, streaming DMA is more widespread used than coherent DMA in kernel, so why can't videobuf2 support streaming dma? :-) You know under some situation, coherent DMA buffer is very slower than other buffer to be accessed by CPU. > >> >> > >> >> > Your current implementation also abuses the design and api of videobuf2 >> >> > memory >> >> > allocators. If the allocator needs to return a custom structure to the >> >> > driver >> >> >> >> I think returning vaddr is enough. >> >> >> >> > you should use cookie method. vaddr is intended to provide only a >> >> > pointer to >> >> > kernel virtual mapping, but you pass a struct page * there. >> >> >> >> No, __get_free_pages returns virtual address instead of 'struct page *'. >> > >> > Then you MUST use cookie for it. vaddr method should return kernel virtual >> > address >> > to the buffer video data. Some parts of videobuf2 relies on this - it is >> > used by file >> > io emulator (read(), write() calls) and mmap equivalent for non-mmu >> > systems. >> > >> > Manual casting in the driver is also a bad idea, that's why there are >> > helper functions >> >> I don't see any casts are needed. The dma address can be got from vaddr with >> dma_map_* easily in drivers, see the usage on patch 8/8(media: video: >> introduce >> omap4 face detection module driver). > > Sorry, but I won't accept such driver/allocator which abuses the defined API. > I've already > pointed what vaddr method is used for. Sorry, could you describe the abuse problem in a bit detail? > >> > defined for both dma_contig and dma_sg allocators: >> > vb2_dma_contig_plane_dma_addr() and >> > vb2_dma_sg_plane_desc(). >> >> These two helpers are not needed and won't be provided by VIDEOBUF2_PAGE >> memops. > > I gave the example. Your allocator should have something similar. I don't think the two helper are required for the VIDEOBUF2_PAGE memops, why can't driver handle DMA mapping directly? thanks, -- Ming Lei -- 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: MEM2MEM devices: how to handle sequence number?
Marek Szyprowski wrote:
>Hello,
>
>On Friday, December 23, 2011 12:29 PM Laurent Pinchart wrote:
>> On Friday 23 December 2011 08:09:25 Marek Szyprowski wrote:
>> > On Thursday, December 22, 2011 3:34 PM Javier Martin wrote:
>> > > we have a processing chain composed of three v4l2 devices:
>> > >
>> > > - ---
>> > > --
>> > >
>> > > | v4l2 source || v4l2 fixer | |
>v4l2
>> > > | encoder |
>> > > |
>> > > | (capture) |-->| (mem2mem)| >|
>(mem2mem) |
>> > >
>> > > >
>> > >
>> > > |___|||
>||
>> > >
>> > > "v4l2 source" generates consecutive sequence numbers so that we
>can
>> > > detect whether a frame has been lost or not.
>> > > "v4l2 fixer" and "v4l2 encoder" cannot lose frames because they
>don't
>> > > interact with an external sensor.
>> > >
>> > > How should "v4l2 fixer" and "v4l2 encoder" behave regarding frame
>> > > sequence number? Should they just copy the sequence number from
>the
>> > > input buffer to the output buffer or should they maintain their
>own
>> > > count for the CAPTURE queue?
>> >
>> > IMHO mem2mem devices, which process buffers in 1:1 way (there is
>always
>> > exactly one 'capture'/destination buffer for every 'output'/source
>buffer)
>> > can simply copy the sequence number from the source buffer to the
>> > destination.
>> >
>> > If there is no such 1:1 mapping between the buffers, drivers should
>> > maintain their own numbers. video encoder is probably an example of
>such
>> > device. A single destination ('capture') buffer with encoded video
>data
>> > might contain a fraction, one or more source ('output') video
>> > buffers/frames.
>> >
>> > > If the former option is chosen we should apply a patch like the
>> > > following so that the sequence number of the input buffer is
>passed to
>> > > the videobuf2 layer:
>> > >
>> > > diff --git a/drivers/media/video/videobuf2-core.c
>> > > b/drivers/media/video/videobuf2-core.c
>> > > index 1250662..7d8a88b 100644
>> > > --- a/drivers/media/video/videobuf2-core.c
>> > > +++ b/drivers/media/video/videobuf2-core.c
>> > > @@ -1127,6 +1127,7 @@ int vb2_qbuf(struct vb2_queue *q, struct
>> > > v4l2_buffer *b)
>> > > */
>> > > list_add_tail(&vb->queued_entry, &q->queued_list);
>> > > vb->state = VB2_BUF_STATE_QUEUED;
>> > > + vb->v4l2_buf.sequence = b->sequence;
>> > > /*
>> > > * If already streaming, give the buffer to driver for
>> > > processing.
>> >
>> > Right, such patch is definitely needed. Please resend it with
>> > 'signed-off-by' annotation.
>>
>> I'm not too sure about that. Isn't the sequence number supposed to be
>ignored
>> by drivers on video output devices ? The documentation is a bit terse
>on the
>> subject, all it says is
>>
>> __u32 sequence Set by the driver, counting the frames in the
>sequence.
>
>We can also update the documentation if needed. IMHO copying sequence
>number
>in mem2mem case if there is 1:1 relation between the buffers is a good
>idea.
>
>Best regards
>--
>Marek Szyprowski
>Samsung Poland R&D Center
>
>
>--
>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
Could there ever be a case where the v4l2 source changes and causes a jump in
the frame count at the encoder, which would then matter to an application?
Regards,
Andy
--
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: MEM2MEM devices: how to handle sequence number?
Hi Marek,
On Friday 23 December 2011 12:35:09 Marek Szyprowski wrote:
> On Friday, December 23, 2011 12:29 PM Laurent Pinchart wrote:
> > On Friday 23 December 2011 08:09:25 Marek Szyprowski wrote:
> > > On Thursday, December 22, 2011 3:34 PM Javier Martin wrote:
> > > > we have a processing chain composed of three v4l2 devices:
> > > >
> > > > - ---
> > > > --
> > > >
> > > > | v4l2 source || v4l2 fixer | |
> > > > | v4l2 encoder |
> > > > |
> > > > | (capture) |-->| (mem2mem)| >| (mem2mem)
> > > > | |
> > > >
> > > > >
> > > >
> > > > |___||| ||
> > > >
> > > > "v4l2 source" generates consecutive sequence numbers so that we can
> > > > detect whether a frame has been lost or not.
> > > > "v4l2 fixer" and "v4l2 encoder" cannot lose frames because they don't
> > > > interact with an external sensor.
> > > >
> > > > How should "v4l2 fixer" and "v4l2 encoder" behave regarding frame
> > > > sequence number? Should they just copy the sequence number from the
> > > > input buffer to the output buffer or should they maintain their own
> > > > count for the CAPTURE queue?
> > >
> > > IMHO mem2mem devices, which process buffers in 1:1 way (there is always
> > > exactly one 'capture'/destination buffer for every 'output'/source
> > > buffer) can simply copy the sequence number from the source buffer to
> > > the destination.
> > >
> > > If there is no such 1:1 mapping between the buffers, drivers should
> > > maintain their own numbers. video encoder is probably an example of
> > > such device. A single destination ('capture') buffer with encoded
> > > video data might contain a fraction, one or more source ('output')
> > > video
> > > buffers/frames.
> > >
> > > > If the former option is chosen we should apply a patch like the
> > > > following so that the sequence number of the input buffer is passed
> > > > to the videobuf2 layer:
> > > >
> > > > diff --git a/drivers/media/video/videobuf2-core.c
> > > > b/drivers/media/video/videobuf2-core.c
> > > > index 1250662..7d8a88b 100644
> > > > --- a/drivers/media/video/videobuf2-core.c
> > > > +++ b/drivers/media/video/videobuf2-core.c
> > > > @@ -1127,6 +1127,7 @@ int vb2_qbuf(struct vb2_queue *q, struct
> > > > v4l2_buffer *b)
> > > >
> > > > */
> > > >
> > > > list_add_tail(&vb->queued_entry, &q->queued_list);
> > > > vb->state = VB2_BUF_STATE_QUEUED;
> > > >
> > > > + vb->v4l2_buf.sequence = b->sequence;
> > > >
> > > > /*
> > > >
> > > > * If already streaming, give the buffer to driver for
> > > > processing.
> > >
> > > Right, such patch is definitely needed. Please resend it with
> > > 'signed-off-by' annotation.
> >
> > I'm not too sure about that. Isn't the sequence number supposed to be
> > ignored by drivers on video output devices ? The documentation is a bit
> > terse on the subject, all it says is
> >
> > __u32 sequence Set by the driver, counting the frames in the
> > sequence.
>
> We can also update the documentation if needed. IMHO copying sequence
> number in mem2mem case if there is 1:1 relation between the buffers is a
> good idea.
My point is that sequence numbers are currently not applicable to video output
devices, at least according to the documentation. Applications will just set
them to 0.
I think it would be better to have the m2m driver set the sequence number
internally on the video output node by incrementing an counter, and pass it
down the pipeline to the video capture node.
--
Regards,
Laurent Pinchart
--
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: MEM2MEM devices: how to handle sequence number?
Hello,
On Friday, December 23, 2011 12:29 PM Laurent Pinchart wrote:
> On Friday 23 December 2011 08:09:25 Marek Szyprowski wrote:
> > On Thursday, December 22, 2011 3:34 PM Javier Martin wrote:
> > > we have a processing chain composed of three v4l2 devices:
> > >
> > > - ---
> > > --
> > >
> > > | v4l2 source || v4l2 fixer | | v4l2
> > > | encoder |
> > > |
> > > | (capture) |-->| (mem2mem)| >| (mem2mem) |
> > >
> > > >
> > >
> > > |___||| ||
> > >
> > > "v4l2 source" generates consecutive sequence numbers so that we can
> > > detect whether a frame has been lost or not.
> > > "v4l2 fixer" and "v4l2 encoder" cannot lose frames because they don't
> > > interact with an external sensor.
> > >
> > > How should "v4l2 fixer" and "v4l2 encoder" behave regarding frame
> > > sequence number? Should they just copy the sequence number from the
> > > input buffer to the output buffer or should they maintain their own
> > > count for the CAPTURE queue?
> >
> > IMHO mem2mem devices, which process buffers in 1:1 way (there is always
> > exactly one 'capture'/destination buffer for every 'output'/source buffer)
> > can simply copy the sequence number from the source buffer to the
> > destination.
> >
> > If there is no such 1:1 mapping between the buffers, drivers should
> > maintain their own numbers. video encoder is probably an example of such
> > device. A single destination ('capture') buffer with encoded video data
> > might contain a fraction, one or more source ('output') video
> > buffers/frames.
> >
> > > If the former option is chosen we should apply a patch like the
> > > following so that the sequence number of the input buffer is passed to
> > > the videobuf2 layer:
> > >
> > > diff --git a/drivers/media/video/videobuf2-core.c
> > > b/drivers/media/video/videobuf2-core.c
> > > index 1250662..7d8a88b 100644
> > > --- a/drivers/media/video/videobuf2-core.c
> > > +++ b/drivers/media/video/videobuf2-core.c
> > > @@ -1127,6 +1127,7 @@ int vb2_qbuf(struct vb2_queue *q, struct
> > > v4l2_buffer *b)
> > > */
> > > list_add_tail(&vb->queued_entry, &q->queued_list);
> > > vb->state = VB2_BUF_STATE_QUEUED;
> > > + vb->v4l2_buf.sequence = b->sequence;
> > > /*
> > > * If already streaming, give the buffer to driver for
> > > processing.
> >
> > Right, such patch is definitely needed. Please resend it with
> > 'signed-off-by' annotation.
>
> I'm not too sure about that. Isn't the sequence number supposed to be ignored
> by drivers on video output devices ? The documentation is a bit terse on the
> subject, all it says is
>
> __u32 sequence Set by the driver, counting the frames in the sequence.
We can also update the documentation if needed. IMHO copying sequence number
in mem2mem case if there is 1:1 relation between the buffers is a good idea.
Best regards
--
Marek Szyprowski
Samsung Poland R&D Center
--
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: MEM2MEM devices: how to handle sequence number?
Hi Marek,
On Friday 23 December 2011 08:09:25 Marek Szyprowski wrote:
> On Thursday, December 22, 2011 3:34 PM Javier Martin wrote:
> > we have a processing chain composed of three v4l2 devices:
> >
> > - ---
> > --
> >
> > | v4l2 source || v4l2 fixer | | v4l2
> > | encoder |
> > |
> > | (capture) |-->| (mem2mem)| >| (mem2mem) |
> >
> > >
> >
> > |___||| ||
> >
> > "v4l2 source" generates consecutive sequence numbers so that we can
> > detect whether a frame has been lost or not.
> > "v4l2 fixer" and "v4l2 encoder" cannot lose frames because they don't
> > interact with an external sensor.
> >
> > How should "v4l2 fixer" and "v4l2 encoder" behave regarding frame
> > sequence number? Should they just copy the sequence number from the
> > input buffer to the output buffer or should they maintain their own
> > count for the CAPTURE queue?
>
> IMHO mem2mem devices, which process buffers in 1:1 way (there is always
> exactly one 'capture'/destination buffer for every 'output'/source buffer)
> can simply copy the sequence number from the source buffer to the
> destination.
>
> If there is no such 1:1 mapping between the buffers, drivers should
> maintain their own numbers. video encoder is probably an example of such
> device. A single destination ('capture') buffer with encoded video data
> might contain a fraction, one or more source ('output') video
> buffers/frames.
>
> > If the former option is chosen we should apply a patch like the
> > following so that the sequence number of the input buffer is passed to
> > the videobuf2 layer:
> >
> > diff --git a/drivers/media/video/videobuf2-core.c
> > b/drivers/media/video/videobuf2-core.c
> > index 1250662..7d8a88b 100644
> > --- a/drivers/media/video/videobuf2-core.c
> > +++ b/drivers/media/video/videobuf2-core.c
> > @@ -1127,6 +1127,7 @@ int vb2_qbuf(struct vb2_queue *q, struct
> > v4l2_buffer *b)
> > */
> > list_add_tail(&vb->queued_entry, &q->queued_list);
> > vb->state = VB2_BUF_STATE_QUEUED;
> > + vb->v4l2_buf.sequence = b->sequence;
> > /*
> > * If already streaming, give the buffer to driver for
> > processing.
>
> Right, such patch is definitely needed. Please resend it with
> 'signed-off-by' annotation.
I'm not too sure about that. Isn't the sequence number supposed to be ignored
by drivers on video output devices ? The documentation is a bit terse on the
subject, all it says is
__u32 sequence Set by the driver, counting the frames in the sequence.
--
Regards,
Laurent Pinchart
--
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: [RFCv1] add DTMB support for DVB API
On 22-12-2011 19:30, Antti Palosaari wrote:
> Rename DMB-TH to DTMB.
Patrick seems to believe that CTTB is a better name. In any case,
whatever name we pick, I think that the DocBook specs (and
maybe a comment at the header file) should point all the known
ways to call this standard. So, I'm fine with any choice.
> Add few new values for existing parameters.
>
> Add two new parameters, interleaving and carrier.
> DTMB supports interleavers: 240 and 720.
> DTMB supports carriers: 1 and 3780.
The API change looks sane to my eyes. I have just a couple
comments below.
>
> Signed-off-by: Antti Palosaari
> ---
> drivers/media/dvb/dvb-core/dvb_frontend.c | 19 ++-
> drivers/media/dvb/dvb-core/dvb_frontend.h |3 +++
> include/linux/dvb/frontend.h | 13 +++--
> include/linux/dvb/version.h |2 +-
> 4 files changed, 33 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/media/dvb/dvb-core/dvb_frontend.c
> b/drivers/media/dvb/dvb-core/dvb_frontend.c
> index 821b225..ec2cbae 100644
> --- a/drivers/media/dvb/dvb-core/dvb_frontend.c
> +++ b/drivers/media/dvb/dvb-core/dvb_frontend.c
> @@ -924,6 +924,8 @@ static struct dtv_cmds_h dtv_cmds[DTV_MAX_COMMAND + 1] = {
> _DTV_CMD(DTV_CODE_RATE_LP, 1, 0),
> _DTV_CMD(DTV_GUARD_INTERVAL, 1, 0),
> _DTV_CMD(DTV_TRANSMISSION_MODE, 1, 0),
> +_DTV_CMD(DTV_CARRIER, 1, 0),
> +_DTV_CMD(DTV_INTERLEAVING, 1, 0),
>
> _DTV_CMD(DTV_ISDBT_PARTIAL_RECEPTION, 1, 0),
> _DTV_CMD(DTV_ISDBT_SOUND_BROADCASTING, 1, 0),
> @@ -974,6 +976,8 @@ static struct dtv_cmds_h dtv_cmds[DTV_MAX_COMMAND + 1] = {
> _DTV_CMD(DTV_GUARD_INTERVAL, 0, 0),
> _DTV_CMD(DTV_TRANSMISSION_MODE, 0, 0),
> _DTV_CMD(DTV_HIERARCHY, 0, 0),
> +_DTV_CMD(DTV_CARRIER, 0, 0),
> +_DTV_CMD(DTV_INTERLEAVING, 0, 0),
>
> _DTV_CMD(DTV_ENUM_DELSYS, 0, 0),
> };
> @@ -1162,7 +1166,8 @@ static void dtv_property_adv_params_sync(struct
> dvb_frontend *fe)
>
> /* Fake out a generic DVB-T request so we pass validation in the ioctl */
> if ((c->delivery_system == SYS_ISDBT) ||
> -(c->delivery_system == SYS_DVBT2)) {
> +(c->delivery_system == SYS_DVBT2) ||
> +(c->delivery_system == SYS_DTMB)) {
> p->u.ofdm.constellation = QAM_AUTO;
> p->u.ofdm.code_rate_HP = FEC_AUTO;
> p->u.ofdm.code_rate_LP = FEC_AUTO;
> @@ -1378,6 +1383,12 @@ static int dtv_property_process_get(struct
> dvb_frontend *fe,
> case DTV_DVBT2_PLP_ID:
> tvp->u.data = c->dvbt2_plp_id;
> break;
> +case DTV_CARRIER:
> +tvp->u.data = c->carrier;
> +break;
> +case DTV_INTERLEAVING:
> +tvp->u.data = c->interleaving;
> +break;
> default:
> return -EINVAL;
> }
> @@ -1544,6 +1555,12 @@ static int dtv_property_process_set(struct
> dvb_frontend *fe,
> case DTV_DVBT2_PLP_ID:
> c->dvbt2_plp_id = tvp->u.data;
> break;
> +case DTV_CARRIER:
> +c->carrier = tvp->u.data;
> +break;
> +case DTV_INTERLEAVING:
> +c->interleaving = tvp->u.data;
> +break;
> default:
> return -EINVAL;
> }
> diff --git a/drivers/media/dvb/dvb-core/dvb_frontend.h
> b/drivers/media/dvb/dvb-core/dvb_frontend.h
> index 67bbfa7..4979ffc 100644
> --- a/drivers/media/dvb/dvb-core/dvb_frontend.h
> +++ b/drivers/media/dvb/dvb-core/dvb_frontend.h
> @@ -343,6 +343,9 @@ struct dtv_frontend_properties {
>
> fe_delivery_system_tdelivery_system;
>
> +u32interleaving;
> +u32carrier;
> +
> /* ISDB-T specifics */
> u8isdbt_partial_reception;
> u8isdbt_sb_mode;
> diff --git a/include/linux/dvb/frontend.h b/include/linux/dvb/frontend.h
> index cb114f5..2fa3bc5 100644
> --- a/include/linux/dvb/frontend.h
> +++ b/include/linux/dvb/frontend.h
> @@ -152,6 +152,7 @@ typedef enum fe_code_rate {
> FEC_AUTO,
> FEC_3_5,
> FEC_9_10,
> +FEC_2_5,
> } fe_code_rate_t;
>
>
> @@ -169,8 +170,11 @@ typedef enum fe_modulation {
> APSK_16,
> APSK_32,
> DQPSK,
> +QAM_4_NR,
While the NR is generally associated with the modulation,
this is a channel code, and not part of the modulation itself
(btw, the DVBv3 API calls it as "constellation").
Ok, it is easier to add it here, but maybe it would be safer
to add a separate field for channel coding that would be used
to enable or disable the Nordstrom-Robinson code.
This is currently used only with QAM-4, but nothing prevents this
parity code to be added to other types of modulation.
> } fe_modulation_t;
>
> +#define QAM_4 QPSK
IMHO, this is overkill, but I'm ok with that.
> +
> typedef enum fe_transmit_mode {
> TRANSMISSION_MODE_2K,
> TRANSMISSION_MODE_8K,
> @@ -201,6 +205,9 @@ typedef enum fe_guard_interval {
> GUARD_INTERVAL_1_128,
> GUARD_INTERVAL_19_128,
> GUARD_INTERVAL_19_256,
> +GUARD_INTERVAL_PN420,
>
Re: [PATCH v2] media i.MX27 camera: Fix field_count handling.
On 23 December 2011 10:07, Guennadi Liakhovetski wrote: > On Fri, 23 Dec 2011, javier Martin wrote: > >> Hi Guennadi, >> thank you for your comments. >> >> On 23 December 2011 00:17, Guennadi Liakhovetski >> wrote: >> > On Thu, 22 Dec 2011, Javier Martin wrote: >> > >> >> To properly detect frame loss the driver must keep >> >> track of a frame_count. >> >> >> >> Furthermore, field_count use was erroneous because >> >> in progressive format this must be incremented twice. >> > >> > Hm, sorry, why this? I just looked at vivi.c - the version before >> > videobuf2 conversion - and it seems to only increment the count by one. >> >> If you look at the videobuf-core code you'll notice that the value >> assigned to v4l2_buf sequence field is (field_count >> 1): > > Right, i.e., field-count / 2. So, it really only counts _frames_, not > fields, doesn't it? > Yes, v4l2_buf sequence field counts _frames_ but field_count counts _fields_, that's why I increment field-count twice in my driver. -- Javier Martin Vista Silicon S.L. CDTUC - FASE C - Oficina S-345 Avda de los Castros s/n 39005- Santander. Cantabria. Spain +34 942 25 32 60 www.vista-silicon.com -- 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 PATCH v2 4/8] media: videobuf2: introduce VIDEOBUF2_PAGE memops
Hello, On Friday, December 23, 2011 10:51 AM Ming Lei wrote: > On Fri, Dec 23, 2011 at 5:34 PM, Marek Szyprowski > wrote: > > >> For example, on ARM, there is very limited kernel virtual address space > >> reserved > >> for DMA coherent buffer mapping, the default size is about 2M if I > >> don't remember mistakenly. > > > > It can be easily increased for particular boards, there is no problem with > > this. > > It is not easily to increase it because there is very limited space reserved > for > this purpose, see Documentation/arm/memory.txt. Also looks like it is > not configurable. It is really not a big issue to increase it by a few MBytes. > >> > I understand that there might be some speed issues with coherent > >> > (uncached) > >> > userspace mappings, but I would solve it in completely different way. > >> > The interface > >> > >> Also there is poor performance inside kernel space, see [1] > > > > Your driver doesn't access video data inside kernel space, so this is also > > not an issue. > > Why not introduce it so that other drivers(include face detection) can > benefit with it? :-) We can get back into this once a driver which really benefits from comes. > >> > > >> > Your current implementation also abuses the design and api of videobuf2 > >> > memory > >> > allocators. If the allocator needs to return a custom structure to the > >> > driver > >> > >> I think returning vaddr is enough. > >> > >> > you should use cookie method. vaddr is intended to provide only a > >> > pointer to > >> > kernel virtual mapping, but you pass a struct page * there. > >> > >> No, __get_free_pages returns virtual address instead of 'struct page *'. > > > > Then you MUST use cookie for it. vaddr method should return kernel virtual > > address > > to the buffer video data. Some parts of videobuf2 relies on this - it is > > used by file > > io emulator (read(), write() calls) and mmap equivalent for non-mmu systems. > > > > Manual casting in the driver is also a bad idea, that's why there are > > helper functions > > I don't see any casts are needed. The dma address can be got from vaddr with > dma_map_* easily in drivers, see the usage on patch 8/8(media: video: > introduce > omap4 face detection module driver). Sorry, but I won't accept such driver/allocator which abuses the defined API. I've already pointed what vaddr method is used for. > > defined for both dma_contig and dma_sg allocators: > > vb2_dma_contig_plane_dma_addr() and > > vb2_dma_sg_plane_desc(). > > These two helpers are not needed and won't be provided by VIDEOBUF2_PAGE > memops. I gave the example. Your allocator should have something similar. Best regards -- Marek Szyprowski Samsung Poland R&D Center -- 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: [Linaro-mm-sig] [RFC v3 0/2] Introduce DMA buffer sharing mechanism
On Wed, Dec 21, 2011 at 1:50 AM, Dave Airlie wrote: >> >> I think this is a really good v1 version of dma_buf. It contains all the >> required bits (with well-specified semantics in the doc patch) to >> implement some basic use-cases and start fleshing out the integration with >> various subsystem (like drm and v4l). All the things still under >> discussion like >> - userspace mmap support >> - more advanced (and more strictly specified) coherency models >> - and shared infrastructure for implementing exporters >> are imo much clearer once we have a few example drivers at hand and a >> better understanding of some of the insane corner cases we need to be able >> to handle. >> >> And I think any risk that the resulting clarifications will break a basic >> use-case is really minimal, so I think it'd be great if this could go into >> 3.3 (maybe as some kind of staging/experimental infrastructure). >> >> Hence for both patches: >> Reviewed-by: Daniel Vetter > > Yeah I'm with Daniel, I like this one, I can definitely build the drm > buffer sharing layer on top of this. > > How do we see this getting merged? I'm quite happy to push it to Linus > if we don't have an identified path, though it could go via a Linaro > tree as well. > > so feel free to add: > Reviewed-by: Dave Airlie Thanks Daniel and Dave! I guess we can start with staging for 3.3, and see how it shapes up. I will post the latest patch version pretty soon. Arnd, Dave: do you have any preference on the path it takes to get merged? In my mind, Linaro tree might make more sense, but I would leave it upto you gentlemen. > > Dave. Best regards, ~Sumit. -- 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: [Linaro-mm-sig] [RFC v2 1/2] dma-buf: Introduce dma buffer sharing mechanism
On Wed, Dec 21, 2011 at 10:57 PM, Arnd Bergmann wrote: > On Tuesday 20 December 2011, Daniel Vetter wrote: >> > I'm thinking for a first version, we can get enough mileage out of it by >> > saying: >> > 1) only exporter can mmap to userspace >> > 2) only importers that do not need CPU access to buffer.. Thanks Rob - and the exporter can do the mmap outside of dma-buf usage, right? I mean, we don't need to provide an mmap to dma_buf() and restrict it to exporter, when the exporter has more 'control' of the buffer anyways. > BR, ~Sumit. -- 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 v3 1/2] dma-buf: Introduce dma buffer sharing mechanism
Hi Konrad,
On Tue, Dec 20, 2011 at 10:06 PM, Konrad Rzeszutek Wilk
wrote:
> On Mon, Dec 19, 2011 at 02:03:30PM +0530, Sumit Semwal wrote:
>> This is the first step in defining a dma buffer sharing mechanism.
>>
>> A new buffer object dma_buf is added, with operations and API to allow easy
>> sharing of this buffer object across devices.
>>
>> The framework allows:
>> - different devices to 'attach' themselves to this buffer, to facilitate
>> backing storage negotiation, using dma_buf_attach() API.
>
> Any thoughts of adding facility to track them? So you can see who is using
> what?
Not for version 1, but it would be a useful addition once we start
using this mechanism.
>
>> - association of a file pointer with each user-buffer and associated
>> allocator-defined operations on that buffer. This operation is called the
>> 'export' operation.
>
> 'create'? or 'alloc' ?
>
> export implies an import somwhere and I don't think that is the case here.
I will rephrase it as suggested by Rob as well.
>
>> - this exported buffer-object to be shared with the other entity by asking
>> for
>> its 'file-descriptor (fd)', and sharing the fd across.
>> - a received fd to get the buffer object back, where it can be accessed using
>> the associated exporter-defined operations.
>> - the exporter and user to share the scatterlist using map_dma_buf and
>> unmap_dma_buf operations.
>>
>> Atleast one 'attach()' call is required to be made prior to calling the
>> map_dma_buf() operation.
>
> for the whole memory region or just for the device itself?
Rob has very eloquently and kindly explained it in his reply.
>
>>
>> +/*
>> + * is_dma_buf_file - Check if struct file* is associated with dma_buf
>> + */
>> +static inline int is_dma_buf_file(struct file *file)
>> +{
>> + return file->f_op == &dma_buf_fops;
>> +}
>> +
>> +/**
>
> Wrong kerneldoc.
I looked into scripts/kernel-doc, and
Documentation/kernel-doc-na-HOWTO.txt => both these places mention
that the kernel-doc comments have to start with /**, and I couldn't
spot an error in what's wrong with my usage - would you please
elaborate on what you think is not right?
>
>> +/**
>> + * struct dma_buf_attachment - holds device-buffer attachment data
>
> OK, but what is the purpose of it?
I will add that in the comments.
>
>> + * @dmabuf: buffer for this attachment.
>> + * @dev: device attached to the buffer.
> ^^^ this
>> + * @node: list_head to allow manipulation of list of dma_buf_attachment.
>
> Just say: "list of dma_buf_attachment"'
ok.
>
>> + * @priv: exporter-specific attachment data.
>
> That "exporter-specific.." brings to my mind custom decleration forms. But
> maybe that is me.
:) well, in context of dma-buf 'exporter', it makes sense.
>
>> + */
>> +struct dma_buf_attachment {
>> + struct dma_buf *dmabuf;
>> + struct device *dev;
>> + struct list_head node;
>> + void *priv;
>> +};
>
> Why don't you move the decleration of this below 'struct dma_buf'?
> It would easier than to read this structure..
I could do that, but then anyways I will have to do a
forward-declaration of dma_buf_attachment, since I have to use it in
dma_buf_ops. If it improves readability, I am happy to move it below
struct dma_buf.
>
>> +
>> +/**
>> + * struct dma_buf_ops - operations possible on struct dma_buf
>> + * @attach: allows different devices to 'attach' themselves to the given
>
> register?
>> + * buffer. It might return -EBUSY to signal that backing storage
>> + * is already allocated and incompatible with the requirements
>
> Wait.. allocated or attached?
This and above comment on 'register' are already answered by Rob in
his explanation of the sequence in earlier reply. [the Documentation
patch [2/2] also tries to explain it]
>
>> + * of requesting device. [optional]
>
> What is optional? The return value? Or the 'attach' call? If the later , say
> that in the first paragraph.
>
ok, sure. it is meant for the attach op.
>
>> + * @detach: detach a given device from this buffer. [optional]
>> + * @map_dma_buf: returns list of scatter pages allocated, increases usecount
>> + * of the buffer. Requires atleast one attach to be called
>> + * before. Returned sg list should already be mapped into
>> + * _device_ address space. This call may sleep. May also return
>
> Ok, there is some __might_sleep macro you should put on the function.
>
That's a nice suggestion; I will add it to the wrapper function for
map_dma_buf().
>> + * -EINTR.
>
> Ok. What is the return code if attach has _not_ been called?
Will document it to return -EINVAL if atleast on attach() hasn't been called.
>
>> + * @unmap_dma_buf: decreases usecount of buffer, might deallocate scatter
>> + * pages.
>> + * @release: release this buffer; to be called after the last dma_buf_put.
>> + * @sync_sg_for_cpu: sync the sg list for cpu.
>> + * @sync_sg_for_device: synch the sg list for
Re: [RFC PATCH v2 4/8] media: videobuf2: introduce VIDEOBUF2_PAGE memops
Hi, On Fri, Dec 23, 2011 at 5:34 PM, Marek Szyprowski wrote: >> For example, on ARM, there is very limited kernel virtual address space >> reserved >> for DMA coherent buffer mapping, the default size is about 2M if I >> don't remember mistakenly. > > It can be easily increased for particular boards, there is no problem with > this. It is not easily to increase it because there is very limited space reserved for this purpose, see Documentation/arm/memory.txt. Also looks like it is not configurable. > >> > I understand that there might be some speed issues with coherent (uncached) >> > userspace mappings, but I would solve it in completely different way. The >> > interface >> >> Also there is poor performance inside kernel space, see [1] > > Your driver doesn't access video data inside kernel space, so this is also > not an issue. Why not introduce it so that other drivers(include face detection) can benefit with it? :-) >> > >> > Your current implementation also abuses the design and api of videobuf2 >> > memory >> > allocators. If the allocator needs to return a custom structure to the >> > driver >> >> I think returning vaddr is enough. >> >> > you should use cookie method. vaddr is intended to provide only a pointer >> > to >> > kernel virtual mapping, but you pass a struct page * there. >> >> No, __get_free_pages returns virtual address instead of 'struct page *'. > > Then you MUST use cookie for it. vaddr method should return kernel virtual > address > to the buffer video data. Some parts of videobuf2 relies on this - it is used > by file > io emulator (read(), write() calls) and mmap equivalent for non-mmu systems. > > Manual casting in the driver is also a bad idea, that's why there are helper > functions I don't see any casts are needed. The dma address can be got from vaddr with dma_map_* easily in drivers, see the usage on patch 8/8(media: video: introduce omap4 face detection module driver). > defined for both dma_contig and dma_sg allocators: > vb2_dma_contig_plane_dma_addr() and > vb2_dma_sg_plane_desc(). These two helpers are not needed and won't be provided by VIDEOBUF2_PAGE memops. thanks, -- Ming Lei -- 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 PATCH v2 4/8] media: videobuf2: introduce VIDEOBUF2_PAGE memops
Hello, On Friday, December 23, 2011 10:22 AM Ming Lei wrote: > On Thu, Dec 22, 2011 at 5:28 PM, Marek Szyprowski > wrote: > >> DMA contig memory resource is very limited and precious, also > >> accessing to it from CPU is very slow on some platform. > >> > >> For some cases(such as the comming face detection driver), DMA Streaming > >> buffer is enough, so introduce VIDEOBUF2_PAGE to allocate continuous > >> physical memory but letting video device driver to handle DMA buffer > >> mapping > >> and unmapping things. > >> > >> Signed-off-by: Ming Lei > > > > Could you elaborate a bit why do you think that DMA contig memory resource > > is so limited? If dma_alloc_coherent fails because of the memory > > fragmentation, > > the alloc_pages() call with order > 0 will also fail. > > For example, on ARM, there is very limited kernel virtual address space > reserved > for DMA coherent buffer mapping, the default size is about 2M if I > don't remember mistakenly. It can be easily increased for particular boards, there is no problem with this. > > I understand that there might be some speed issues with coherent (uncached) > > userspace mappings, but I would solve it in completely different way. The > > interface > > Also there is poor performance inside kernel space, see [1] Your driver doesn't access video data inside kernel space, so this is also not an issue. > > for both coherent/uncached and non-coherent/cached contig allocator should > > be the > > same, so exchanging them is easy and will not require changes in the driver. > > I'm planning to introduce some design changes in memory allocator api and > > introduce > > prepare and finish callbacks in allocator ops. I hope to post the rfc after > > Christmas. For your face detection driver using standard dma-contig > > allocator > > shouldn't be a big issue. > > > > Your current implementation also abuses the design and api of videobuf2 > > memory > > allocators. If the allocator needs to return a custom structure to the > > driver > > I think returning vaddr is enough. > > > you should use cookie method. vaddr is intended to provide only a pointer to > > kernel virtual mapping, but you pass a struct page * there. > > No, __get_free_pages returns virtual address instead of 'struct page *'. Then you MUST use cookie for it. vaddr method should return kernel virtual address to the buffer video data. Some parts of videobuf2 relies on this - it is used by file io emulator (read(), write() calls) and mmap equivalent for non-mmu systems. Manual casting in the driver is also a bad idea, that's why there are helper functions defined for both dma_contig and dma_sg allocators: vb2_dma_contig_plane_dma_addr() and vb2_dma_sg_plane_desc(). Best regards -- Marek Szyprowski Samsung Poland R&D Center -- 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 PATCH v2 4/8] media: videobuf2: introduce VIDEOBUF2_PAGE memops
On Thu, Dec 22, 2011 at 5:28 PM, Marek Szyprowski wrote: > Hello, > > On Wednesday, December 14, 2011 3:00 PM Ming Lei wrote: > >> DMA contig memory resource is very limited and precious, also >> accessing to it from CPU is very slow on some platform. >> >> For some cases(such as the comming face detection driver), DMA Streaming >> buffer is enough, so introduce VIDEOBUF2_PAGE to allocate continuous >> physical memory but letting video device driver to handle DMA buffer mapping >> and unmapping things. >> >> Signed-off-by: Ming Lei > > Could you elaborate a bit why do you think that DMA contig memory resource > is so limited? If dma_alloc_coherent fails because of the memory > fragmentation, > the alloc_pages() call with order > 0 will also fail. For example, on ARM, there is very limited kernel virtual address space reserved for DMA coherent buffer mapping, the default size is about 2M if I don't remember mistakenly. > > I understand that there might be some speed issues with coherent (uncached) > userspace mappings, but I would solve it in completely different way. The > interface Also there is poor performance inside kernel space, see [1] > for both coherent/uncached and non-coherent/cached contig allocator should be > the > same, so exchanging them is easy and will not require changes in the driver. > I'm planning to introduce some design changes in memory allocator api and > introduce > prepare and finish callbacks in allocator ops. I hope to post the rfc after > Christmas. For your face detection driver using standard dma-contig allocator > shouldn't be a big issue. > > Your current implementation also abuses the design and api of videobuf2 memory > allocators. If the allocator needs to return a custom structure to the driver I think returning vaddr is enough. > you should use cookie method. vaddr is intended to provide only a pointer to > kernel virtual mapping, but you pass a struct page * there. No, __get_free_pages returns virtual address instead of 'struct page *'. thanks, -- Ming Lei [1], http://marc.info/?t=13119814851&r=1&w=2 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] media i.MX27 camera: Fix field_count handling.
On Fri, 23 Dec 2011, javier Martin wrote:
> Hi Guennadi,
> thank you for your comments.
>
> On 23 December 2011 00:17, Guennadi Liakhovetski
> wrote:
> > On Thu, 22 Dec 2011, Javier Martin wrote:
> >
> >> To properly detect frame loss the driver must keep
> >> track of a frame_count.
> >>
> >> Furthermore, field_count use was erroneous because
> >> in progressive format this must be incremented twice.
> >
> > Hm, sorry, why this? I just looked at vivi.c - the version before
> > videobuf2 conversion - and it seems to only increment the count by one.
>
> If you look at the videobuf-core code you'll notice that the value
> assigned to v4l2_buf sequence field is (field_count >> 1):
Right, i.e., field-count / 2. So, it really only counts _frames_, not
fields, doesn't it?
Thanks
Guennadi
> http://lxr.linux.no/#linux+v3.1.6/drivers/media/video/videobuf-core.c#L370
>
> Since mx2_camera driver only supports video formats which are either
> progressive or provide both fields in the same buffer, this
> "field_count" must be incremented twice so that the final sequence
> number is OK.
>
> >>
> >> Signed-off-by: Javier Martin
> >> ---
> >> drivers/media/video/mx2_camera.c | 5 -
> >> 1 files changed, 4 insertions(+), 1 deletions(-)
> >>
> >> diff --git a/drivers/media/video/mx2_camera.c
> >> b/drivers/media/video/mx2_camera.c
> >> index ea1f4dc..ca76dd2 100644
> >> --- a/drivers/media/video/mx2_camera.c
> >> +++ b/drivers/media/video/mx2_camera.c
> >> @@ -255,6 +255,7 @@ struct mx2_camera_dev {
> >> dma_addr_t discard_buffer_dma;
> >> size_t discard_size;
> >> struct mx2_fmt_cfg *emma_prp;
> >> + u32 frame_count;
> >
> > The rule I usually follow, when choosing variable type is the following:
> > does it really have to be fixed bit-width? The positive reply is pretty
> > rare, it comes mostly if (a) the variable is used to store values read
> > from or written to some (fixed-width) hardware registers, or (b) the
> > variable belongs to a fixed ABI, that has to be the same on different
> > (32-bit, 64-bit) systems, like (arguably) ioctl()s, data, transferred over
> > the network or stored on a medium (filesystems,...). This doesn't seem to
> > be the case here, so, I would just use an (unsigned) int.
>
> Thanks for the tip. I hadn't thought of it that way. I just saw that
> v4l2_buf.sequence was a __u32 and I thought it was convenient to use
> the same type for this variable which is closely related to it
>
> Anyway, let me send a second version of the patch since I've just
> noticed this one doesn't reflect lost frames in the field_count field.
>
> --
> Javier Martin
> Vista Silicon S.L.
> CDTUC - FASE C - Oficina S-345
> Avda de los Castros s/n
> 39005- Santander. Cantabria. Spain
> +34 942 25 32 60
> www.vista-silicon.com
>
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
--
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: Add tuner_type to zl10353 config and use it for reporting signal directly from tuner.
On 12/23/2011 10:14 AM, Antti Palosaari wrote: On 12/23/2011 01:45 AM, Miroslav Slugeň wrote: This patch is wrong, please use 8971 instead. Could you explain which is wrong? Your old code or that new override version I explained? fe->ops.read_signal_strength = fe->ops.tuner_ops.get_rf_strength; aargh, surely you are speaking patchwork IDs... my mistake. Antti -- http://palosaari.fi/ -- 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: Add tuner_type to zl10353 config and use it for reporting signal directly from tuner.
On 12/23/2011 01:45 AM, Miroslav Slugeň wrote: This patch is wrong, please use 8971 instead. Could you explain which is wrong? Your old code or that new override version I explained? fe->ops.read_signal_strength = fe->ops.tuner_ops.get_rf_strength; Antti -- http://palosaari.fi/ -- 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
