Re: [PATCH] gpu: ipu-v3: Allow negative offsets for interlaced scanning

[Krzysztof Hałasa]

Steve Longerbeam  writes:

> Right, the selection of interweave is moved to the capture devices,
> so the following will enable interweave:
>
> v4l2-ctl -dN --set-fmt-video=field=interlaced_tb

and

> So the patch to adv7180 needs to be modified to report # field lines.
>
> Try the following:
>
> --- a/drivers/media/i2c/adv7180.c
> +++ b/drivers/media/i2c/adv7180.c

With this patch, fix-csi-interlaced.3 seems to work for me.
"ipu2_csi1":2 reports [fmt:AYUV32/720x576 field:seq-tb], but the
/dev/videoX shows (when requested) 720 x 576 NV12 interlaced, top field
first, and I'm getting valid output.

Thanks for your work.
-- 
Krzysztof Halasa

Industrial Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland

Re: [PATCH] gpu: ipu-v3: Allow negative offsets for interlaced scanning

[Krzysztof Hałasa]

Reporting from the field :-)

The fix-csi-interlaced.3 branch is still a bit off the track I guess:

   media-ctl -V "'adv7180 2-0020':0 [fmt:UYVY2X8/576 field:seq-tb]"
   media-ctl -V "'ipu2_csi1_mux':2 [fmt:UYVY2X8/720x576]"
   media-ctl -V "'ipu2_csi1':2 [fmt:AYUV32/720x576 field:interlaced-tb]"

does:
"adv7180 2-0020":0 [fmt:UYVY2X8/720x576 field:alternate]
"ipu2_csi1_mux":1  [fmt:UYVY2X8/720x576 field:alternate]
"ipu2_csi1_mux":2  [fmt:UYVY2X8/720x576 field:alternate]
"ipu2_csi1":0  [fmt:UYVY2X8/720x576 field:alternate
 crop.bounds:(0,0)/720x1152
 crop:(0,0)/720x1152
 compose.bounds:(0,0)/720x1152
 compose:(0,0)/720x1152]
"ipu2_csi1":2  [fmt:AYUV32/720x1152 field:seq-tb]

... and not interlaced[-*], as with fix-csi-interlaced.2.

The double heights are funny, too - probably an ADV7180 issue.
-- 
Krzysztof Halasa

Industrial Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland

Re: [PATCH v2 04/10] media: imx: interweave only for sequential input/interlaced output fields

[Krzysztof Hałasa]

Steve Longerbeam  writes:

> One final note, it is incorrect to assign 'seq-tb' to a PAL signal according
> to this new understanding. Because according to various sites (for example
> [1]), both standard definition NTSC and PAL are bottom field dominant,
> so 'seq-tb' is not correct for PAL.

Actually this isn't the case:

- real PAL (= analog) is (was) interlaced, so you could choose any
  "dominant field" and it would work fine (stuff originating as film
  movies being an exception).

- the general idea at these times was that NTSC-style digital video was
  bottom-first while PAL-style was top-first.

- for example, most (practically all?) commercial TV-style interlaced
  PAL DVDs were top-first (movies were usually progressive).

- standard TV (DVB 576i) uses (used) top-first as well.

- this seems to be confirmed by e.g. ITU-R REC-BR.469-7-2002 (annex 1).
  Hovewer, this suggests that field 1 is the top one and 2 is bottom
  (and not first and second in terms of time).

- if field 1 = top and 2 = bottom indeed, then we're back at BT.656 and
  my original idea that the SAV/EAV codes show straigh the so called
  odd/even lines and not some magic, standard-dependent lines of first
  and second fields. This needs to be verified.
  I think the ADV7180 forces the SAV/EAV codes (one can't set them
  depending od PAL/NTSC etc), so we could assume it does it right.

- a notable exception to PAL = top first rule was DV and similar stuff
  (the casette camcorder things, including Digital8, miniDV, and
  probably derivatives). DV (including PAL) used bottom-first
  universally.

I think we should stick to default PAL=TB, NTSC=BT rule, though the user
should be able to override it (if working with e.g. PAL DV camcorder
connected with an analog cable - not very probable, I guess).
-- 
Krzysztof Halasa

Industrial Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland

Re: [PATCH v2 04/10] media: imx: interweave only for sequential input/interlaced output fields

[Krzysztof Hałasa]

Steve Longerbeam  writes:

> Yes, I had already implemented this idea yesterday, I've added it
> to branch fix-csi-interlaced.3. The CSI will swap field capture
> (field 1 first, then field 2, by inverting F bit in CCIR registers) if
> the field order input to the CSI is different from the requested
> output field order.

It seems the fix-csi-interlaced.2 was a bit better.

Now I do:
media-ctl -V "'adv7180 2-0020':0 [fmt:UYVY2X8/720x576 field:alternate]"
media-ctl -V "'ipu2_csi1_mux':2 [fmt:UYVY2X8/720x576]"
media-ctl -V "'ipu2_csi1':2 [fmt:AYUV32/720x576 field:interlaced]"

and get:
"adv7180 2-0020":0 [fmt:UYVY2X8/720x576 field:alternate]
"ipu2_csi1_mux":1  [fmt:UYVY2X8/720x576 field:alternate]
"ipu2_csi1_mux":2  [fmt:UYVY2X8/720x576 field:alternate]
"ipu2_csi1":0  [fmt:UYVY2X8/720x576 field:alternate]
"ipu2_csi1":2  [fmt:AYUV32/720x576 field:seq-tb]

Needless to say, the output isn't an interlaced frame.
-- 
Krzysztof Halasa

Industrial Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland

Re: [PATCH v2 04/10] media: imx: interweave only for sequential input/interlaced output fields

[Krzysztof Hałasa]

Steve Longerbeam  writes:

> I don't follow you, yes the interweaving step only has access to
> a single frame, but why would interweave need access to another
> frame to carry out seq-bt -> interlaced-tb ? See below...

You can't to that.
You can delay the input stream (skip one field) so the bottom-first
becomes top-first (or top-first - bottom-first), probably with some loss
of chroma quality, but you can't reorder odd and even lines.

To convert (anything)-bt -> (anything)-tb you need two consecutive
fields, the top one and then the bottom one. If the input is *-bt, this
means two "frames" (if the word "frame" is applicable at this point).

CCIR_CODE_* registers are fine, though. They don't change the geometry,
the just skip a single field (sort of, actually they sync to the
required field).
-- 
Krzysztof Halasa

Industrial Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland

Re: [PATCH v2 04/10] media: imx: interweave only for sequential input/interlaced output fields

[Krzysztof Hałasa]

Philipp Zabel  writes:

> I'm probably misunderstanding you, so at the risk of overexplaining:
> There is no way we can ever produce a correct interlaced-tb frame in
> memory from a seq-bt frame output by the CSI, as the interweaving step
> only has access to a single frame.

Anyway we can use CCIR_CODE registers to get the fields in required
order. Actually I think it's the preferred way, even if there are
others.
-- 
Krzysztof Halasa

Industrial Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland

Re: [PATCH v2 01/10] media: imx-csi: Pass sink pad field to ipu_csi_init_interface

[Krzysztof Hałasa]

Steve Longerbeam  writes:

> I think you misunderstood me. Of course there is a first and second
> field. By first I am referring to the first field transmitted, which could
> be top or bottom.

Right. I was thinking the fields are even and odd, but that's not
actually the case (I mean, the numbering uses field 1 and 2 and not
E/O).

> Progressive sensors have no fields, the entire image is captured at
> once as you said.

There are progressive cameras with analog PAL/NTSC output. The signal
is obviously interlaced and consists of fields.
-- 
Krzysztof Halasa

Industrial Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland

Re: i.MX6 IPU CSI analog video input on Ventana

[Krzysztof Hałasa]

I've just tested the PAL setup: in currect situation (v4.17 + Steve's
fix-csi-interlaced.2 + "media: adv7180: fix field type" + a small cheap
PAL camera) the following produces bottom-first interlaced frames:

media-ctl -r -l '"adv7180 2-0020":0->"ipu2_csi1_mux":1[1],
 "ipu2_csi1_mux":2->"ipu2_csi1":0[1],
 "ipu2_csi1":2->"ipu2_csi1 capture":0[1]'

media-ctl -V "'adv7180 2-0020':0 [fmt:UYVY2X8/720x576 field:alternate]"
media-ctl -V "'ipu2_csi1_mux':2 [fmt:UYVY2X8/720x576]"
media-ctl -V "'ipu2_csi1':2 [fmt:AYUV32/720x576 field:interlaced]"

"adv7180 2-0020":0 [fmt:UYVY2X8/720x576 field:alternate]
"ipu2_csi1_mux":1  [fmt:UYVY2X8/720x576 field:alternate]
"ipu2_csi1_mux":2  [fmt:UYVY2X8/720x576 field:alternate]
"ipu2_csi1":0  [fmt:UYVY2X8/720x576 field:alternate]
"ipu2_csi1":2  [fmt:AYUV32/720x576 field:interlaced]

I think it would be great if these changes make their way upstream.
The details could be refined then.
-- 
Krzysztof Halasa
Industrial Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland

Re: i.MX6 IPU CSI analog video input on Ventana

[Krzysztof Hałasa]

Steve Longerbeam  writes:

> I think this must be legacy code from a Freescale BSP requirement
> that the CSI must always capture in T-B order. We should remove this
> code, so that the CSI always captures field 0 followed by field 1,
> irrespective
> of field affinity,

Well it now seems we could do just that.
-- 
Krzysztof Halasa

Industrial Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland

Re: i.MX6 IPU CSI analog video input on Ventana

[Krzysztof Hałasa]

Hi Philipp,

Philipp Zabel  writes:

> My understanding is that the CCIR codes for height == 480 (NTSC)
> currently capture the second field (top) first, assuming that for NTSC
> the EAV/SAV codes are bottom-field-first.

2a38014: 010D07DF 00040596

SA  EA SB  EB  SB  EB
D07DF: 001 101 () 011 111 011 111 (field 0)
40596: 000 100 () 010 110 010 110 (field 1)

The codes apparently are 1=EAV (0=SAV), field#, 1=blanking.
Now BT.656 doesn't say a word about top and bottom fields. There are
just fields 1 and 2. So yes, the CCIR_CODE* registers currently seem to
swap the fields. It also depends on the ADV7180 sending correct codes
based on the even/odd analog fields. Interesting.

> So the CSI captures SEQ_TB for both PAL and NTSC: The three-bit values
> in CCIR_CODE_2/3 are in H,V,F order, and the NTSC case has F=1 for the
> field that is captured first, where F=1 is the field that is marked as
> second field on the wire, so top. Which means that the captured frame
> has two fields captured across frame boundaries, which might be
> problematic if the source data was originally progressive.

Exactly.
Especially if the complete frame is then passed straight to the display,
with the user treating it as progressive (which it isn't anymore).

>>  My guess is the CSI is skipping
>> the first incomplete line (half-line - the first visible line has full
>> length) and BT becomes TB.
>
> That wouldn't make BT TB though, if we'd still capture the bottom field
> (minus its first half line) first?

Well, the entire frame would shift up a line, the bottom "field" would
become top and vice versa. This would effectively make BT->TB and TB->BT.

>> It seems writing to the CCIR_CODE_[12] registers does the trick, though
>> (the captured frames aren't correct and have the lines swapped in pairs
>> because t/b field pointers aren't correctly set).
>
> What are you writing exactly? 0x01040596 to CCIR_CODE_1 and 0x000d07df
> to CCIR_CODE_2?

Yes.

> That is what I would expect to capture SEQ_BT for NTSC
> data, and the IPU could interweave this into INTERLACED_BT, correctly if
> we fix ipu_cpmem_interlaced_scan to allow negative offsets.

Exactly.
-- 
Krzysztof Halasa

Industrial Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland

Re: [PATCH v2 10/10] media: imx.rst: Update doc to reflect fixes to interlaced capture

[Krzysztof Hałasa]

Steve Longerbeam  writes:

> Hmm, if the sink is 'alternate', and the requested source is
> 'interlaced*', perhaps we should allow the source to be
> 'interlaced*' and not override it. For example, if requested
> is 'interlaced-tb', let it be that. IOW assume user knows something
> we don't about the original field order, or is experimenting
> with finding the correct field order.

Yes, this is clearly possible. In fact the analog signal doesn't carry
information about the field order (if any). The video editing/encoding
software does motion estimation for this, and there is no other way
(given that the video material can change from progressive to interlaced
and vice versa, and probably can change the field order, at any time).
-- 
Krzysztof Halasa

Industrial Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland

Re: [PATCH v2 04/10] media: imx: interweave only for sequential input/interlaced output fields

[Krzysztof Hałasa]

Philipp Zabel  writes:

> This is ok in this patch, but we can't use this check in the following
> TRY_FMT patch as there is no way to interweave
> SEQ_TB -> INTERLACED_BT (because in SEQ_TB the B field is newer than T,
> but in INTERLACED_BT it has to be older) or SEQ_BT -> INTERLACED_TB (the
> other way around).

Actually we can do SEQ_TB -> INTERLACED_BT and SEQ_BT -> INTERLACED_TB
rather easily. We only need to skip a single field at start :-)
That's what CCIR_CODE_* registers do.

To be honest, SEQ_TB and SEQ_BT are precisely the same thing
(i.e., SEQUENTIAL). It's up to the user to say which field is the first.
There is the progressive sensor exception, though, and the TB/BT could
be a hint for downstream elements (i.e., setting the default field
order).

But I think we should be able to request INTERLACED_TB or INTERLACED_BT
(with any analog signal on input) and the CCIR_CODE registers should be
set accordingly. This should all magically work fine.
-- 
Krzysztof Halasa

Industrial Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland

Re: [PATCH v2 01/10] media: imx-csi: Pass sink pad field to ipu_csi_init_interface

[Krzysztof Hałasa]

Steve Longerbeam  writes:

> I think we should return to enforcing field order to userspace that
> matches field order from the source, which is what I had implemented
> previously. I agree with you that we should put off allowing inverting
> field order.

There is no any particular field order at the source, most of the time.
The odd field is followed by the even field, and so on, sure. But there
is no "first" and "second" field, any field can be the "first".

The exception to this is a camera with a progressive sensor - both
"fields" are taken at the same time and transmitted one after the other,
so in this case the order is defined (by the camera, e.g. B-T on DV even
with PAL version). But this isn't exactly PAL/NTSC.
-- 
Krzysztof Halasa

Industrial Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland

Re: i.MX6 IPU CSI analog video input on Ventana

[Krzysztof Hałasa]

Steve Longerbeam  writes:

> I tend to agree, I've found conflicting info out there regarding
> PAL vs. NTSC field order. And I've never liked having to guess
> at input analog standard based on input # lines. I will go ahead
> and remove the field order override code.

I've merged your current fix-csi-interlaced.2 branch (2018-06-01
00:06:45 UTC 22ad9f30454b6e46979edf6f8122243591910a3e) along with
"media: adv7180: fix field type" commit and NTSC camera:

media-ctl -V "'adv7180 2-0020':0 [fmt:UYVY2X8/720x480 field:alternate]"
media-ctl -V "'ipu2_csi1_mux':2 [fmt:UYVY2X8/720x480]"
media-ctl -V "'ipu2_csi1':2 [fmt:AYUV32/720x480 field:interlaced/-bt/-tb]"

correctly sets:

"adv7180 2-0020":0 [fmt:UYVY2X8/720x480 field:alternate]
"ipu2_csi1_mux":1  [fmt:UYVY2X8/720x480 field:alternate]
"ipu2_csi1_mux":2  [fmt:UYVY2X8/720x480 field:alternate]
"ipu2_csi1":0  [fmt:UYVY2X8/720x480 field:alternate]
"ipu2_csi1":2  [fmt:AYUV32/720x480 field:interlaced/-bt/-tb]

but all 3 cases seem to produce top-first interlaced frames.
The CCIR_CODE_* register dump shows no differences:
2a38014: 010D07DF 00040596 00FF

...it's because the code in drivers/gpu/ipu-v3/ipu-csi.c still sets the
registers depending on the height of the image. Hovewer, I'm using 480
lines here, so it should be B-T instead. My guess is the CSI is skipping
the first incomplete line (half-line - the first visible line has full
length) and BT becomes TB.

It seems writing to the CCIR_CODE_[12] registers does the trick, though
(the captured frames aren't correct and have the lines swapped in pairs
because t/b field pointers aren't correctly set).
-- 
Krzysztof Halasa

Industrial Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland

Re: i.MX6 IPU CSI analog video input on Ventana

[Krzysztof Hałasa]

Philipp Zabel  writes:

> Note that the code in ipu_csi_init_interface currently hard-codes field
> order depending on frame size. It could be that selecting opposite field
> order is as easy as switching the relevant parts of writes to registers
> CCIR_CODE_2 and _3, but we'd have to pass the desired output field order
> to this function. I'd welcome if somebody would verify that this works.

I can test anything I guess.
Though, in this case, I would be surprised if there is something else
needed. We already do the opposite field order by switching the
CCIR_CODE_[23] values :-)
-- 
Krzysztof Halasa

Industrial Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland

Re: i.MX6 IPU CSI analog video input on Ventana

[Krzysztof Hałasa]

Steve Longerbeam  writes:

>> but it should be possible for the user to explicitly request the field
>> order on CSI output (I can make a patch I guess).
>
> If you think that is the correct behavior, I will remove the override
> code. I suppose it makes sense to allow user to select field order even
> if that order does not make sense given the input standard. I'm fine
> either way, Philipp what is your opinion? I'll go with the popular vote :)

I think it should be up to the user.
Actually, PAL and NTSC aren't valid names in the digital world. Their
meaning ends in the ADV7180 (or equivalent). I don't know if PAL and/or
NTSC specify the field order in the analog frame (meaningful when
someone hooks a camera with progressive sensor and analog, interlaced
output), but the digital YUV422 from ADV to CSI isn't NTSC/PAL anymore.
It's just WxH @ framerate + possible interlacing, sequential fields,
top-bottom or otherwise, etc. The analog standard names could be used
here but just as defaults.

If we were strict (and we don't want to force it), then we should set
NTSC/PAL thing on ADV7180 input, 720x480@29.97i (or 720x576@50i, or
704x... etc) on the input parts of the CSI/IPU (where there are no video
frames yet), and 720x480@29.97i B-T or T-B (or default, or separate
fields - whatever suits the user) on the output from CSI.

I remember the reversed field order was sometimes needed - for example,
PAL DV (the casette camcorder thing) produced B-T frames (same as NTSC),
and to avoid (slight) additional quality loss one had to process it
(up to e.g. .MP4, DVD, and then to HDMI, SCART etc) as B-T.
It wasn't a problem in otherwise-PAL-centric environment.
-- 
Krzysztof Halasa

Industrial Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland

Re: i.MX6 IPU CSI analog video input on Ventana

[Krzysztof Hałasa]

Steve Longerbeam  writes:

> Yes, you'll need to patch adv7180.c to select either
> 'seq-bt/tb' or 'alternate'. The current version will override
> any attempt to set field to anything other than 'interlaced'.
> This is in anticipation of getting a patch merged for adv7180
> that fixes this.

Right. I've applied the patch from your adv718x-v6 branch (just the
"media: adv7180: fix field type" patch) and now it works.

Also, I have changed "seq-bt" to "alternate" (in the examples in
Documentation/media/v4l-drivers/imx.rst) - the data stream from ADV7180
to CSI consists of separate fields which can then be merged into frames
in any order requested by the user (e.g. in accordance with "digital PAL
/ NTSC" requirements).

The following:
media-ctl -V "'adv7180 2-0020':0 [fmt:UYVY2X8/720x480 field:alternate]"
media-ctl -V "'ipu2_csi1_mux':2 [fmt:UYVY2X8/720x480]"
media-ctl -V "'ipu2_csi1':2 [fmt:AYUV32/720x480 field:interlaced]"
now produces:

"adv7180 2-0020":0 [fmt:UYVY2X8/720x480 field:alternate]
"ipu2_csi1_mux":1  [fmt:UYVY2X8/720x480 field:alternate]
"ipu2_csi1_mux":2  [fmt:UYVY2X8/720x480 field:alternate]
"ipu2_csi1":0  [fmt:UYVY2X8/720x480 field:alternate]
"ipu2_csi1":2  [fmt:AYUV32/720x480 field:interlaced-bt]

and it works correctly.

The only issue is that I can't:
media-ctl -V "'ipu2_csi1':2 [fmt:AYUV32/720x480 field:interlaced-tb]"
(it remains fixed in -bt mode since NTSC is the default). I think we may
set TB/BT by default (depending on CSI input geometry or TV standard),
but it should be possible for the user to explicitly request the field
order on CSI output (I can make a patch I guess).
-- 
Krzysztof Halasa

Industrial Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland

Re: i.MX6 IPU CSI analog video input on Ventana

[Krzysztof Hałasa]

Hi Steve,

Steve Longerbeam  writes:

> Krzysztof, in the meantime the patches are available in my
> media-tree fork, for testing on the Ventana GW5300:
>
> g...@github.com:slongerbeam/mediatree.git, branch 'fix-csi-interlaced'

I assume fix-csi-interlaced.2 is a newer version, isn't it?

I merged it and I think I can't set the correct config:
media-ctl -r -l '"adv7180 2-0020":0->"ipu2_csi1_mux":1[1],
 "ipu2_csi1_mux":2->"ipu2_csi1":0[1],
 "ipu2_csi1":2->"ipu2_csi1 capture":0[1]'

media-ctl -V "'adv7180 2-0020':0 [fmt:UYVY2X8/720x480 field:seq-bt]"
media-ctl -V "'ipu2_csi1_mux':2 [fmt:UYVY2X8/720x480]"
media-ctl -V "'ipu2_csi1':2 [fmt:AYUV32/720x480 field:interlaced]"

produces:
"adv7180 2-0020":0 [fmt:UYVY2X8/720x480 field:interlaced]
"ipu2_csi1_mux":1  [fmt:UYVY2X8/720x480 field:interlaced]
"ipu2_csi1_mux":2  [fmt:UYVY2X8/720x480 field:interlaced]
"ipu2_csi1":0  [fmt:UYVY2X8/720x480 field:interlaced]
"ipu2_csi1":2  [fmt:AYUV32/720x480  field:interlaced]

Do I need to patch ADV7180 for field type "sequential"?

It seems setting seq-bt on ADV7180 sets "interlaced" on ADV -> MUX input
-> MUX output. Setting "'ipu2_csi1_mux':2 [fmt:UYVY2X8/720x480]" sets
interlaced on all elements of the pipeline. The effect is a pair of
fields, not an interlaced frame.
-- 
Krzysztof Halasa

Industrial Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland

Re: i.MX6 IPU CSI analog video input on Ventana

[Krzysztof Hałasa]

Philipp Zabel  writes:

> Maybe scanline interlave and double write reduction can't be used at the
> same time?

Well, if it works in non-interlaced modes - it may be the case.

Perhaps the data reduction is done before the field merge step. This
would make it incompatible: in interlaced mode we need all color data
from a field (we could potentially remove all color info from the other
field, or use some average).
-- 
Krzysztof Halasa

Industrial Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland

Re: i.MX6 IPU CSI analog video input on Ventana

[Krzysztof Hałasa]

Steve Longerbeam  writes:

>> The manual says: Reduce Double Read or Writes (RDRW):
>> This bit is relevant for YUV4:2:0 formats. For write channels:
>> U and V components are not written to odd rows.
>>
>> How could it be so? With YUV420, are they normally written?
>
> Well, given that this bit exists, and assuming I understand it
> correctly (1),
> I guess the U and V components for odd rows normally are placed on the
> AXI bus. Which is a total waste of bus bandwidth because in 4:2:0,
> the U and V components are the same for odd and even rows.

Right. Now, the AXI bus is just a "memory bus", it's a newer version of
the AHB. One can't simply "place data" on AXI, it must be a write to
a specific address, and the data will end up in RAM (assuming the
configuration is sane). How can we have two possible data formats (with
and without the RDRW bit) with fixed image format (420-type) is beyond
me.

> Commits
>
> 14330d7f08 ("media: imx: csi: enable double write reduction")
> b54a5c2dc8 ("media: imx: prpencvf: enable double write reduction")
>
> should be reverted for now, until the behavior of this bit is better
> understood.

I agree.

I have dumped a raw frame (720 x 480 NV12 frame size 518400 from
interlaced NTSC camera), with the RDRW bit set.

The Y plane contains, well, valid Y data (720 x 480 bytes).

The color plane (360 pixels x 240 line pairs * 2 colors) has every other
line pair zeroed. I.e., there is a 720-byte line pair filled with valid UV
data, then there are 720 zeros (360 zeroed UV pairs). Then there is valid
UV data and so on.

Not sure what could it be for. Some weird sort of YUV 4:1:0? I guess we
don't want it ATM.

WRT ADV7180 field format:

> This might be a good time to bring up the fact that the ADV7180 driver
> is wrong
> to set output to "interlaced". The ADV7180 does not transmit top lines
> interlaced
> with bottom lines. It transmits all top lines followed by all bottom
> lines (or
> vice-versa), i.e. it should be either V4L2_FIELD_SEQ_TB or
> V4L2_FIELD_SEQ_BT.
> It can also be set to V4L2_FIELD_ALTERNATE, and then it is left up to
> downstream
> elements to determine field order (TB or BT).

Right. ADV7180, AFAIK, doesn't have the hardware (frame buffer) to get
two interlaced fields and merge them to form a complete frame.
It simply transforms the incoming analog signal into binary data stream.
This issue should be fixed.

Thanks for your work,
-- 
Krzysztof Halasa

Industrial Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland

Re: i.MX6 IPU CSI analog video input on Ventana

[Krzysztof Hałasa]

Steve Longerbeam  writes:

> Sorry I did find a bug. Please try this patch:

Ok, your patch fixes the first problem (sets the CSI interlaced mode
on input when field = NOE is requested on output). Posting in full since
your mail came somehow mangled with UTF-8.

--- a/drivers/staging/media/imx/imx-media-csi.c
+++ b/drivers/staging/media/imx/imx-media-csi.c
@@ -629,7 +629,6 @@ static int csi_setup(struct csi_priv *priv)
 {
struct v4l2_mbus_framefmt *infmt, *outfmt;
struct v4l2_mbus_config mbus_cfg;
-   struct v4l2_mbus_framefmt if_fmt;
 
infmt = >format_mbus[CSI_SINK_PAD];
outfmt = >format_mbus[priv->active_output_pad];
@@ -640,20 +639,13 @@ static int csi_setup(struct csi_priv *priv)
priv->upstream_ep.bus.mipi_csi2.flags :
priv->upstream_ep.bus.parallel.flags;
 
-   /*
-* we need to pass input frame to CSI interface, but
-* with translated field type from output format
-*/
-   if_fmt = *infmt;
-   if_fmt.field = outfmt->field;
-
ipu_csi_set_window(priv->csi, >crop);
 
ipu_csi_set_downsize(priv->csi,
 priv->crop.width == 2 * priv->compose.width,
 priv->crop.height == 2 * priv->compose.height);
 
-   ipu_csi_init_interface(priv->csi, _cfg, _fmt);
+   ipu_csi_init_interface(priv->csi, _cfg, infmt);
 
ipu_csi_set_dest(priv->csi, priv->dest);
 

> (the removed code was meant to deal with field type at sink pad being
> "alternate", which ipu_csi_init_interface() doesn't currently recognize, but
> that should be dealt with in IPUv3 driver).

I see.

> With that you should be able to set pad ipu2_csi1:2 to field type
> "none", e.g.
> set pipeline to:
>
> media-ctl -V '"adv7180 2-0020":0[fmt:UYVY2X8 720x480 field:interlaced]'
> media-ctl -V '"ipu2_csi1_mux":1[fmt:UYVY2X8 720x480 field:interlaced]'
> media-ctl -V '"ipu2_csi1_mux":2[fmt:UYVY2X8 720x480 field:interlaced]'
> media-ctl -V '"ipu2_csi1":0[fmt:UYVY2X8 720x480 field:interlaced]'
> media-ctl -V '"ipu2_csi1":2[fmt:UYVY2X8 720x480 field:none]'
>
> With the above patch, capture from ipu1_csi0:2 is fixed for me on
> SabreAuto.

Right, it also works fine for me on Ventana GW5300 (with
ipu_cpmem_skip_odd_chroma_rows() removed as well, of course).

> You may also want to try adding a ~500 msec delay after adv7180 power on
> as I explained earlier:

Ok. In fact I don't have a sync problem even without it, the rolling
image always eventually syncs. Maybe I'll investigate the data stream
(from ADV7180 to CSI) and see what's on. I't a bit complicated since
what I have is just an oscilloscope.
-- 
Krzysztof Halasa

Industrial Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland

Re: i.MX6 IPU CSI analog video input on Ventana

[Krzysztof Hałasa]

Hi,

I've experimented with the ADV7180 a bit and this is what I found.

First, I'm using (with a NTSC camera but I guess PAL won't be much
different):
media-ctl -V '"adv7180 2-0020":0[fmt:UYVY2X8 720x480 field:interlaced]'
media-ctl -V '"ipu2_csi1_mux":1[fmt:UYVY2X8 720x480 field:interlaced]'
media-ctl -V '"ipu2_csi1_mux":2[fmt:UYVY2X8 720x480 field:interlaced]'
media-ctl -V '"ipu2_csi1":0[fmt:UYVY2X8 720x480 field:interlaced]'
media-ctl -V '"ipu2_csi1":2[fmt:UYVY2X8 720x480 field:interlaced]'

IOW I set all of the parts to interlaced mode. If i set the last element
to "none", the CSI is not set for interlaced input, and nothing works at
the low level.

This requires a quick temporary hack:
--- a/drivers/staging/media/imx/imx-media-csi.c
+++ b/drivers/staging/media/imx/imx-media-csi.c
@@ -474,8 +474,8 @@ static int csi_idmac_setup_channel(struct csi_priv *priv)
 
ipu_smfc_set_burstsize(priv->smfc, burst_size);
 
-   if (image.pix.field == V4L2_FIELD_NONE &&
-   V4L2_FIELD_HAS_BOTH(infmt->field))
+   if (1 || (image.pix.field == V4L2_FIELD_NONE &&
+ V4L2_FIELD_HAS_BOTH(infmt->field)))
ipu_cpmem_interlaced_scan(priv->idmac_ch,
  image.pix.bytesperline);
 

I.e., I need to set CPMEM to interlaced mode when I operate CSI in
interlaced mode. The original code is a bit unclear to me in fact.

The following is required as well. Now the question is why we can't skip
writing those odd UV rows. Anyway, with these 2 changes, I get a stable
NTSC (and probably PAL) interlaced video stream.

The manual says: Reduce Double Read or Writes (RDRW):
This bit is relevant for YUV4:2:0 formats. For write channels:
U and V components are not written to odd rows.

How could it be so? With YUV420, are they normally written?
OTOH it seems that not only UV is broken with this bit set.
Y is broken as well.

--- a/drivers/staging/media/imx/imx-media-csi.c
+++ b/drivers/staging/media/imx/imx-media-csi.c
@@ -413,14 +413,12 @@ static int csi_idmac_setup_channel(struct csi_priv *priv)
passthrough_bits = 16;
break;
case V4L2_PIX_FMT_YUV420:
case V4L2_PIX_FMT_NV12:
burst_size = (image.pix.width & 0x3f) ?
 ((image.pix.width & 0x1f) ?
  ((image.pix.width & 0xf) ? 8 : 16) : 32) : 64;
passthrough = is_parallel_16bit_bus(>upstream_ep);
passthrough_bits = 16;
-   /* Skip writing U and V components to odd rows */
-   ipu_cpmem_skip_odd_chroma_rows(priv->idmac_ch);
break;
case V4L2_PIX_FMT_YUYV:
case V4L2_PIX_FMT_UYVY:
-- 
Krzysztof Halasa

Industrial Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland

Re: i.MX6 IPU CSI analog video input on Ventana

[Krzysztof Hałasa]

Hi,

Steve Longerbeam  writes:

> Hi Krzysztof, I've been on vacation, just returned today. I will
> find the time this week to attempt to reproduce your results on
> a SabreAuto quad with the adv7180.

Great. Please let me know if I can assist you somehow.

> Btw, if you just need to capture an interlaced frame (lines 0,1,2,...)
> without motion compensation, there is no need to use the VDIC
> path. Capturing directly from ipu2_csi1 should work, I've tested
> this many times on a SabreAuto. But I will try to reproduce your
> results.

That's what I was thinking. Thanks a lot.
-- 
Krzysztof Halasa

Industrial Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland

Re: i.MX6 IPU CSI analog video input on Ventana

[Krzysztof Hałasa]

Tested with NTSC camera, it's the same as with PAL.
The only case when IPU2_CSI1_SENS_CONF register is set to interlaced
mode (PRCTL=3, CCIR interlaced mode (BT.656)) is when all parts of the
pipeline are set to interlaced:

"adv7180 2-0020":0 [fmt:UYVY2X8/720x576 field:interlaced]
"ipu2_csi1_mux":1  [fmt:UYVY2X8/720x576 field:interlaced]
"ipu2_csi1_mux":2  [fmt:UYVY2X8/720x576 field:interlaced]
"ipu2_csi1":0  [fmt:UYVY2X8/720x576 field:interlaced]
"ipu2_csi1":2  [fmt:AYUV32/720x576 field:interlaced]

The image is stable and in sync, the "only" problem is that I get two
concatenated field images (in one V4L2 frame) instead of a normal
interlaced frame (all lines in order - 0, 1, 2, 3, 4 etc).
IOW I get V4L2_FIELD_ALTERNATE, V4L2_FIELD_SEQ_TB or V4L2_FIELD_SEQ_BT
(the data format, I don't mean the pixel format.field) while I need to
get V4L2_FIELD_INTERLACED, V4L2_FIELD_INTERLACED_TB or _BT.


If I set "ipu2_csi1":2 to field:none, the IPU2_CSI1_SENS_CONF is set to
progressive mode (PRCTL=2). It's the last element of the pipeline I can
configure, it's connected straight to "ipu2_csi1 capture" aka
/dev/videoX. I think CSI can't work with interlaced camera (and ADV7180)
when set to progressive, can it?


I wonder... perhaps to get an interlaced frame I need to route the data
through VDIC (ipu2_vdic, the deinterlacer)?
-- 
Krzysztof Halasa

Industrial Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland

Re: i.MX6 IPU CSI analog video input on Ventana

[Krzysztof Hałasa]

Tim,

Tim Harvey  writes:

> What version of kernel are you using and what specific board model do
> you have (full board model and/or serial number so I know if you've
> got an IMX6DL or an IMX6Q) and have you modified the device-tree? I
> tested the adv7180 a couple of months ago but I don't know if I hit
> your specific combination. I was using NTSC but if your not getting
> VSYNC then yes I would like to make sure the pinmux is correct for
> your situation.

At the moment I'm using 4.16.0 and this particular board IDs itself as
GW5304-D2, the CPU is i.MX6Q. I mostly use GW5300s and GW5400s (and
others in smaller numbers) and they work fine with Steve's older driver
(the one with a special ADV7180 module).

I'm using the official device tree (v4.16 in this case) with just the
LDB (LVDS display) portion removed.

I guess I can test it with a NTSC camera.
-- 
Krzysztof Halasa

Industrial Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland

Re: i.MX6 IPU CSI analog video input on Ventana

[Krzysztof Hałasa]

Steve Longerbeam  writes:

> Yes, the CSI on i.MX6 does not deal well with unstable bt.656 sync codes,
> which results in vertical sync issues (scrolling or split images). The
> ADV7180
> will often shift the sync codes around in various situations (initial
> power on,
> see below, also when there is an interruption of the input analog CVBS
> signal).

I'm not convinced it's the sync code issue. I've compared the key
registers (both 4.2 + your old driver vs 4.16) and this is what I got:

"adv7180 2-0020":0 [fmt:UYVY2X8/720x576 field:interlaced]
"ipu2_csi1_mux":1  [fmt:UYVY2X8/720x576 field:interlaced]
"ipu2_csi1_mux":2  [fmt:UYVY2X8/720x576 field:interlaced]
"ipu2_csi1":0  [fmt:UYVY2X8/720x576 field:interlaced]
"ipu2_csi1":2  [fmt:AYUV32/720x576 field:none]

There is H sync but no V sync. The encoding is wrong (I'm using NV12 but
what I get from /dev/video* isn't NV12).

IPU2_CSI1 registers are:
048 C 10   14   18   1C
2a38000: 04000A20 023F02CF 023F02CF 0  0 00040030  00FF
vs the old driver:
 04000A30 027002CF 023F02CF 0  0 01040596 000D07DF 00FF

0: CSI1 Sensor Configuration (IPUx_CSI1_SENS_CONF)
The new driver uses progressive mode while the old one - interlaced
mode.

4: CSI1 Sense Frame Size Register (IPUx_CSI1_SENS_FRM_SIZE)
The new driver uses 575 lines in place of 624 (this probably needs to be
checked with the ADV7180 docs, though the old version works fine).

14, 18: CSI1 CCIR Code Register 1 and 2 (IPUx_CSI1_CCIR_CODE_[12])
The new driver doesn't use "Error detection and correction" and it seems
the codes are set for progressive mode. I think this can't work.


With:
"adv7180 2-0020":0 [fmt:UYVY2X8/720x576 field:interlaced]
"ipu2_csi1_mux":1  [fmt:UYVY2X8/720x576 field:none]
"ipu2_csi1_mux":2  [fmt:UYVY2X8/720x576 field:none]
"ipu2_csi1":0  [fmt:UYVY2X8/720x576 field:none]
"ipu2_csi1":2  [fmt:AYUV32/720x576 field:none]

Still, V sync but no H sync. The Y/colors are good, except that
there are two consecutive images on the screen.
2a38000: 04000A20 023F02CF 023F02CF 0  0 00040030  00FF
CSI set to progressive again. Setting the registers manually (SENS_CONF
and SAV/EAV codes) makes the image stabilize, though there are still two
images (split in the middle). Apparently something is simply appending
the two field images, instead of merging them properly.


With:
"adv7180 2-0020":0 [fmt:UYVY2X8/720x576 field:interlaced]
"ipu2_csi1_mux":1  [fmt:UYVY2X8/720x576 field:interlaced]
"ipu2_csi1_mux":2  [fmt:UYVY2X8/720x576 field:interlaced]
"ipu2_csi1":0  [fmt:UYVY2X8/720x576 field:interlaced]
"ipu2_csi1":2  [fmt:AYUV32/720x576 field:interlaced]

2a38000: 04000A30 027002CF 023F02CF 0  0 01040596 000D07DF 00FF
the CSI is set for interlaced mode, and there are two stable images
(both fields concatenated).


The first case again (all except ipu2_csi1 set to interlaced). I've
manually set the CSI registers and now the image is synchronized and
stable (one complete frame this time). The problem is it's not NV12
(nor YUV420), the colors are all green and the Y lines comes in pairs -
valid then invalid (probably color) and so on.


Could it be a DTS problem? I'm using imx6q-gw53xx.dtb file,
the 8-bit ADV7180 (40 pin version) is connected to the IPU2 CSI1 DATA,
EIM_EB3 = HSYNC, EIM_A16 = PIXCLK and EIM_D29 = VSYNC. HSYNC and VSYNC
aren't currently used, though.

I Guess I have to compare all IPU registers.
-- 
Krzysztof Halasa

Industrial Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland

Re: i.MX6 IPU CSI analog video input on Ventana

[Krzysztof Hałasa]

Steve Longerbeam  writes:

>> Second, the image format information I'm getting out of "ipu2_csi1
>> capture" device is:
>>
>> open("/dev/video6")
>> ioctl(VIDIOC_S_FMT, {V4L2_BUF_TYPE_VIDEO_CAPTURE,
>>  fmt.pix={704x576, pixelformat=NV12, V4L2_FIELD_INTERLACED} =>
>>  fmt.pix={720x576, pixelformat=NV12, V4L2_FIELD_INTERLACED,
>>  bytesperline=720, sizeimage=622080,
>>  colorspace=V4L2_COLORSPACE_SMPTE170M}})
>>
>> Now, the resulting image obtained via QBUF/DQBUF doesn't seem to be
>> a single interlaced frame (like it was with older drivers). Actually,
>> I'm getting the two fields, encoded with NV12 and concatenated
>> together (I think it's V4L2_FIELD_SEQ_TB or V4L2_FIELD_SEQ_BT).
>>
>> What's wrong?
>
> Set field type at /dev/video6 to NONE. That will enable IDMAC
> interweaving of the top and bottom fields.

Such as this?
"adv7180 2-0020":0
[fmt:UYVY2X8/720x576 field:interlaced]
"ipu2_csi1_mux":1
[fmt:UYVY2X8/720x576 field:interlaced]
"ipu2_csi1_mux":2
[fmt:UYVY2X8/720x576 field:interlaced]
"ipu2_csi1":0
[fmt:UYVY2X8/720x576 field:interlaced
 crop.bounds:(0,0)/720x576
 crop:(0,0)/720x576
 compose.bounds:(0,0)/720x576
 compose:(0,0)/720x576]
"ipu2_csi1":2
[fmt:AYUV32/720x576 field:none]

There is something wrong - the resulting image is out of (vertical)
sync, it seems the time it takes to receive a frame is a bit longer than
the normal 40 ms. I can also set field to NONE on "ipu2_csi1_mux":[12]
but it doesn't sync, either. Only with everything set to INTERLACED, the
frame is synchronized (actually, it starts unsynchronized, but slowly
scrolls down the screen and eventually "catches sync").
With the old drivers nothing like this happens: the image is "instantly"
synchronized and it's a single interlaced frame, not the two halves
concatenated.
-- 
Krzysztof Halasa

Industrial Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland

i.MX6 IPU CSI analog video input on Ventana

[Krzysztof Hałasa]

Hi,

I'm using analog PAL video in on GW53xx/54xx boards (through ADV7180
chip and 8-bit parallel CSI input, with (presumably) BT.656).
I'm trying to upgrade from e.g. Linux 4.2 + Steve's older MX6 camera
driver (which works fine) to v.4.16 with the recently merged driver.

media-ctl -r -l '"adv7180 2-0020":0->"ipu2_csi1_mux":1[1],
 "ipu2_csi1_mux":2->"ipu2_csi1":0[1],
 "ipu2_csi1":2->"ipu2_csi1 capture":0[1]'

media-ctl -V '"adv7180 2-0020":0[fmt:UYVY2X8 720x576 field:interlaced]'
media-ctl -V '"ipu2_csi1_mux":1[fmt:UYVY2X8 720x576 field:interlaced]'
media-ctl -V '"ipu2_csi1_mux":2[fmt:UYVY2X8 720x576 field:interlaced]'

It seems there are issues, though:

First, I can't find a way to change to PAL standard. *s_std() doesn't
propagate from "ipu2_csi1 capture" through "ipu2_csi1_mux" to adv7180.

For now I have just changed the default:
--- a/drivers/media/i2c/adv7180.c
+++ b/drivers/media/i2c/adv7180.c
@@ -1320,7 +1321,7 @@ static int adv7180_probe(struct i2c_client *client,
 
 state->irq = client->irq;
 mutex_init(>mutex);
-state->curr_norm = V4L2_STD_NTSC;
+state->curr_norm = V4L2_STD_PAL;
 if (state->chip_info->flags & ADV7180_FLAG_RESET_POWERED)
 state->powered = true;
 else


Second, the image format information I'm getting out of "ipu2_csi1
capture" device is:

open("/dev/video6")
ioctl(VIDIOC_S_FMT, {V4L2_BUF_TYPE_VIDEO_CAPTURE,
fmt.pix={704x576, pixelformat=NV12, V4L2_FIELD_INTERLACED} =>
fmt.pix={720x576, pixelformat=NV12, V4L2_FIELD_INTERLACED,
bytesperline=720, sizeimage=622080,
colorspace=V4L2_COLORSPACE_SMPTE170M}})

Now, the resulting image obtained via QBUF/DQBUF doesn't seem to be
a single interlaced frame (like it was with older drivers). Actually,
I'm getting the two fields, encoded with NV12 and concatenated
together (I think it's V4L2_FIELD_SEQ_TB or V4L2_FIELD_SEQ_BT).

What's wrong?
Is it possible to get a real V4L2_FIELD_INTERLACED frame, so it can be
passed straight to the CODA H.264 encoder?
-- 
Krzysztof Halasa

Industrial Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland

i.MX6: 16-bit parallel CSI

[Krzysztof Hałasa]

Another thing. I'm using a 16-bit parallel CSI camera (with the so
called BT.1120, the 16-bit version of BT.656). Both BT.1120 and BT.656
send sync info embedded in the pixel data stream. The current code calls
for "passthrough" in 16-bit YCbCr (YUV) mode, though this isn't actually
required in such situation. "Passthrough" seems to be needed only when
the input is "generic data", e.g. in YCbCr (YUV) modes (no matter how
wide) with dedicated sync inputs (also with Bayer, Y and non-standard
stuff).

According to IMX6SDLIEC (i.MX 6Solo/6DualLite Applications Processors
for Industrial Products, 4.11.10.1 IPU Sensor Interface Signal Mapping),
even 20-bit YCbCr mode is supported with the "on-the-fly processing",
though I haven't tested it.

Currently drivers/staging/media/imx/imx-media-csi.c:
/*
 * Check for conditions that require the IPU to handle the
 * data internally as generic data, aka passthrough mode:
 * - raw bayer formats
 * - the sensor bus is 16-bit parallel
 */
@@ -353,19 +353,19 @@ static int csi_idmac_setup_channel(struct csi_priv *priv)
case V4L2_PIX_FMT_NV12:
burst_size = (image.pix.width & 0x3f) ?
 ((image.pix.width & 0x1f) ?
  ((image.pix.width & 0xf) ? 8 : 16) : 32) : 64;
passthrough = (sensor_ep->bus_type != V4L2_MBUS_CSI2 &&
   sensor_ep->bus.parallel.bus_width >= 16);
passthrough_bits = 16;
break;
(and so on)

I wonder how to fix it. Should we simply drop the bus_width test, and
use other means of signalling that the incoming data should be treated
as "generic"? Perhaps it could depend on some mbus_cfg->type
(drivers/gpu/ipu-v3/ipu-csi.c), which is either V4L2_MBUS_PARALLEL,
V4L2_MBUS_BT656 or V4L2_MBUS_CSI2 (should we add V4L2_MBUS_BT1120 here?)
-- 
Krzysztof Halasa

Industrial Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland

Re: [PATCH][MEDIA]i.MX6 CSI: Fix MIPI camera operation in RAW/Bayer mode

[Krzysztof Hałasa]

Fabio Estevam  writes:

>> --- a/drivers/staging/media/imx/imx-media-csi.c
>> +++ b/drivers/staging/media/imx/imx-media-csi.c
>> @@ -340,7 +340,7 @@ static int csi_idmac_setup_channel(struct csi_priv *priv)
>> case V4L2_PIX_FMT_SGBRG8:
>> case V4L2_PIX_FMT_SGRBG8:
>> case V4L2_PIX_FMT_SRGGB8:
>> -   burst_size = 8;
>> +   burst_size = 16;
>> passthrough = true;
>> passthrough_bits = 8;
>> break;
>
> Russell has sent the same fix and Philipp made a comment about the
> possibility of using 32-byte or 64-byte bursts:
> http://driverdev.linuxdriverproject.org/pipermail/driverdev-devel/2017-October/111960.html

Great.

Sometimes I wonder how many people are working on exactly the same
stuff.
-- 
Krzysztof Halasa

Industrial Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland

[PATCH][MEDIA]i.MX6 CSI: Fix MIPI camera operation in RAW/Bayer mode

[Krzysztof Hałasa]

Without this fix, in 8-bit Y/Bayer mode, every other 8-byte group
of pixels is lost.
Not sure about possible side effects, though.

Signed-off-by: Krzysztof Hałasa <khal...@piap.pl>

--- a/drivers/staging/media/imx/imx-media-csi.c
+++ b/drivers/staging/media/imx/imx-media-csi.c
@@ -340,7 +340,7 @@ static int csi_idmac_setup_channel(struct csi_priv *priv)
case V4L2_PIX_FMT_SGBRG8:
case V4L2_PIX_FMT_SGRBG8:
case V4L2_PIX_FMT_SRGGB8:
-   burst_size = 8;
+   burst_size = 16;
passthrough = true;
passthrough_bits = 8;
break;

-- 
Krzysztof Halasa

Industrial Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland

[PATCH][MEDIA] i.MX6: Fix MIPI CSI-2 LP-11 check

[Krzysztof Hałasa]

Bitmask for the MIPI CSI-2 data PHY status doesn't seem to be correct.
Fix it.

Signed-off-by: Krzysztof Hałasa <khal...@piap.pl>

--- a/drivers/staging/media/imx/imx6-mipi-csi2.c
+++ b/drivers/staging/media/imx/imx6-mipi-csi2.c
@@ -252,8 +252,8 @@ static int csi2_dphy_wait_stopstate(struct csi2_dev *csi2)
u32 mask, reg;
int ret;
 
-   mask = PHY_STOPSTATECLK |
-   ((csi2->bus.num_data_lanes - 1) << PHY_STOPSTATEDATA_BIT);
+   mask = PHY_STOPSTATECLK | (((1 << csi2->bus.num_data_lanes) - 1) <<
+  PHY_STOPSTATEDATA_BIT);
 
ret = readl_poll_timeout(csi2->base + CSI2_PHY_STATE, reg,
 (reg & mask) == mask, 0, 50);

-- 
Krzysztof Halasa

Industrial Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland

i.MX6 CODA warning: vb2: counters for queue xxx, buffer y: UNBALANCED!

[Krzysztof Hałasa]

Hi,

I'm using i.MX6 CODA H.264 encoder and found a minor bug somewhere.
Not sure how it should be fixed, though.
The problem manifests itself when I configure (open, qbuf etc) the
encoder device and then close it without any start/stop streaming calls.
I'm using 2 buffers in this example:

vb2:   counters for queue b699c808, buffer 0: UNBALANCED!
vb2: buf_init: 1 buf_cleanup: 1 buf_prepare: 1 buf_finish: 1
vb2: buf_queue: 0 buf_done: 0
vb2: alloc: 1 put: 1 prepare: 1 finish: 0 mmap: 1
 
vb2: get_userptr: 0 put_userptr: 0
vb2: attach_dmabuf: 0 detach_dmabuf: 0 map_dmabuf: 0 unmap_dmabuf: 0
vb2: get_dmabuf: 0 num_users: 0 vaddr: 0 cookie: 0

vb2:   counters for queue b699c808, buffer 1: UNBALANCED!
vb2: buf_init: 1 buf_cleanup: 1 buf_prepare: 1 buf_finish: 1
vb2: buf_queue: 0 buf_done: 0
vb2: alloc: 1 put: 1 prepare: 1 finish: 0 mmap: 1
 
vb2: get_userptr: 0 put_userptr: 0
vb2: attach_dmabuf: 0 detach_dmabuf: 0 map_dmabuf: 0 unmap_dmabuf: 0
vb2: get_dmabuf: 0 num_users: 0 vaddr: 0 cookie: 0

These are H.264 (encoder "capture") buffers. Note the alloc
prepare/finish disparity.

I have investigated a bit and it seems it's some missing *buf_done()
call, probably belonging to coda_release(), but I'm not sure. Or maybe
should my program "finish" the buffers before doing close()?

Linux 4.13. I'm using CONFIG_VIDEO_ADV_DEBUG=y.
Compile with:
arm-pc-linux-gnueabihf-gcc -std=gnu99 -O2 -W -Wall -Wno-sign-compare 
-Wno-missing-field-initializers -D_GNU_SOURCE coda_test.c -o coda_test
Any other details are, of course, available.

Ideas?
--
Krzysztof Halasa

Industrial Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland


// -*- mode: c; c-basic-offset: 4; tab-width: 4; -*-
// coda_test.c
#include 
#include 
#include 
#include 
#include 
#include 
#include 
#include 
#include 

static inline int doioctl(int fd, unsigned long req, void *ptr)
{
return TEMP_FAILURE_RETRY(ioctl(fd, req, ptr));
}

void *xmalloc(size_t size)
{
void *ptr = malloc(size);
if (!ptr) {
printf("Unable to allocate %zu bytes of data\n", size);
exit(1);
}

return ptr;
}

static void print_pixel_format(const char *name, struct v4l2_pix_format *pix)
{
printf("%s %u x %u ", name, pix->width, pix->height);

const char *ptr = (void*)>pixelformat;
for (int i = 0; i < 4; i++) {
if ((ptr[i] >= 'A' && ptr[i] <= 'Z') ||
(ptr[i] >= 'a' && ptr[i] <= 'z') ||
(ptr[i] >= '0' && ptr[i] <= '9'))
printf("%c", ptr[i]);
else
printf("?");
}

switch (pix->field) {
case V4L2_FIELD_NONE:
break;
case V4L2_FIELD_TOP:
printf(" top field");
break;
case V4L2_FIELD_BOTTOM:
printf(" bottom field");
break;
case V4L2_FIELD_INTERLACED:
printf(" interlaced");
break;
case V4L2_FIELD_SEQ_TB:
printf(" top field first");
break;
case V4L2_FIELD_SEQ_BT:
printf(" bottom field first");
break;
default:
printf(" field %u", pix->field);
break;
}
printf(" frame size %u\n", pix->sizeimage);
}

int main(void)
{
struct {
void *start;
size_t length;
} *h264_buffers = NULL;

int fd;

if ((fd = open("/dev/coda-encoder", O_RDWR, 0)) < 0) {
printf("Unable to open video encoder device: %m\n");
return -1;
}

struct v4l2_format encoder_fmt = {
.type = V4L2_BUF_TYPE_VIDEO_OUTPUT,
.fmt.pix.pixelformat = V4L2_PIX_FMT_NV12,
.fmt.pix.width = 704,
.fmt.pix.height = 576,
.fmt.pix.field = V4L2_FIELD_NONE,
};
if (doioctl(fd, VIDIOC_S_FMT, _fmt) < 0) {
printf("Unable to set video encoder sink format\n");
goto close_streams;
}

print_pixel_format("Encoder output:", _fmt.fmt.pix);

encoder_fmt.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
if (doioctl(fd, VIDIOC_G_FMT, _fmt) < 0) {
printf("Unable to get video encoder capture format\n");
goto close_streams;
}

print_pixel_format("Encoder input:", _fmt.fmt.pix);

// Query buffer counts
struct v4l2_requestbuffers req = {
.count = 2,
.type = V4L2_BUF_TYPE_VIDEO_CAPTURE,
.memory = V4L2_MEMORY_MMAP,
};

if (doioctl(fd, VIDIOC_REQBUFS, ) < 0) {
printf("Unable to 

Re: [PATCH] TW686x: Fix OOPS on buffer alloc failure

[Krzysztof Hałasa]

Hans Verkuil  writes:

> Any progress on this? I gather I can expect a new patch from someone?

Well, the issue is trivial and very easy to test, though not present
on common x86 hw. That patch I've sent fixes it, but I'm not the one who
decides.
-- 
Krzysztof Halasa

Industrial Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland

Re: [PATCH] TW686x: Fix OOPS on buffer alloc failure

[Krzysztof Hałasa]

Ezequiel Garcia  writes:

> How about this one (untested) ?
>
> diff --git a/drivers/media/pci/tw686x/tw686x-video.c
> b/drivers/media/pci/tw686x/tw686x-video.c
> index c3fafa97b2d0..77b8d2dbd995 100644
> --- a/drivers/media/pci/tw686x/tw686x-video.c
> +++ b/drivers/media/pci/tw686x/tw686x-video.c
> @@ -86,6 +86,9 @@ static void tw686x_memcpy_dma_free(struct
> tw686x_video_channel *vc,
> struct pci_dev *pci_dev;
> unsigned long flags;
>
> +   /* Make sure this channel is initialized */
> +   if (!dev)
> +   return;

Whatever you wish. Just make sure it doesn't bomb out by default, when
one happens to have such a card in his or her machine.
-- 
Krzysztof Halasa

Industrial Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland

Re: [PATCH] TW686x: Fix OOPS on buffer alloc failure

[Krzysztof Hałasa]

"zhaoxuegang"  writes:

> In my opinion, the oops occur to tw686x_free_dma(struct tw686x_video_channel 
> *vc, unsigned int pb).
> In function tw686x_video_init, if coherent-DMA is not enough, 
> tw686x_alloc_dma will failed.
> Then tw686x_video_free will be called. but some channel's dev is null（in 
> other words, vc->dev is null）

Exactly.
-- 
Krzysztof Halasa

Industrial Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland

Re: [PATCH] TW686x: Fix OOPS on buffer alloc failure

[Krzysztof Hałasa]

Ezequiel Garcia  writes:

>> +   /* Initialize vc->dev and vc->ch for the error path first */
>> +   for (ch = 0; ch < max_channels(dev); ch++) {
>> +   struct tw686x_video_channel *vc = >video_channels[ch];
>> +   vc->dev = dev;
>> +   vc->ch = ch;
>> +   }
>> +
>
> I'm not sure where is the oops this commit fixes, care to explain it to me?

The error path apparently calls tw686x_video_free() which requires
vc->dev to be initialized. Now, the vc->dev is set for the channel being
currently initialized (unsuccesfully), but not for ones which haven't
been initialized yet. tw686x_video_free() iterates over the whole set.

It seems it also happens in "memcpy" mode. I didn't test it before since
on my ARMv7 "memcpy" mode is unusable, it's way too slow. Also, does the
driver attempt to use consistent memory for entire buffers in this mode?
This may work on i686/x86_64 because the caches are coherent by design
and there is no difference between consistent and non-consistent RAM
(if one isn't using SWIOTLB etc).

tw6869: PCI :07:00.0, IRQ 24, MMIO 0x110 (memcpy mode)
tw686x :07:00.0: enabling device (0140 -> 0142)
tw686x :07:00.0: dma0: unable to allocate P-buffer
Unable to handle kernel NULL pointer dereference at virtual address 
PC is at _raw_spin_lock_irqsave+0x10/0x4c
LR is at tw686x_memcpy_dma_free+0x1c/0x124
pc : [<805a8b14>]lr : [<7f04a3c0>]psr: 20010093
sp : be915c80  ip :   fp : bea1b000
r10:   r9 : fff4  r8 : b000
r7 :   r6 : 03f0  r5 :   r4 : bf0e21f8
r3 : 7f04a3a4  r2 :   r1 :   r0 : 20010013
Flags: nzCv  IRQs off  FIQs on  Mode SVC_32  ISA ARM  Segment none
Control: 10c5387d  Table: 4e91804a  DAC: 0051
Process udevd (pid: 88, stack limit = 0xbe914210)
(_raw_spin_lock_irqsave) from (tw686x_memcpy_dma_free+0x1c/0x124)
(tw686x_memcpy_dma_free) from (tw686x_video_free+0x50/0x78)
(tw686x_video_free) from (tw686x_video_init+0x478/0x5e8)
(tw686x_video_init) from (tw686x_probe+0x36c/0x3fc)
(tw686x_probe) from (pci_device_probe+0x88/0xf4)
(pci_device_probe) from (driver_probe_device+0x238/0x2d8)
(driver_probe_device) from (__driver_attach+0xac/0xb0)
(__driver_attach) from (bus_for_each_dev+0x6c/0xa0)
(bus_for_each_dev) from (bus_add_driver+0x1a0/0x218)
(bus_add_driver) from (driver_register+0x78/0xf8)
(driver_register) from (do_one_initcall+0x40/0x168)
(do_one_initcall) from (do_init_module+0x60/0x3a4)
(do_init_module) from (load_module+0x1c90/0x20e4)
(load_module) from (SyS_finit_module+0x8c/0x9c)
(SyS_finit_module) from (ret_fast_syscall+0x0/0x3c)
Code: e1a02000 e10f f10c0080 f592f000 (e1923f9f)


With the patch:
tw6869: PCI :07:00.0, IRQ 24, MMIO 0x110 (memcpy mode)
tw686x :07:00.0: enabling device (0140 -> 0142)
tw686x :07:00.0: dma0: unable to allocate P-buffer
tw686x :07:00.0: can't register video
tw686x: probe of :07:00.0 failed with error -12
--
Krzysztof Halasa

Industrial Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland

[PATCH] TW686x: Fix OOPS on buffer alloc failure

[Krzysztof Hałasa]

Signed-off-by: Krzysztof Hałasa <khal...@piap.pl>

diff --git a/drivers/media/pci/tw686x/tw686x-video.c 
b/drivers/media/pci/tw686x/tw686x-video.c
index c3fafa9..d637f47 100644
--- a/drivers/media/pci/tw686x/tw686x-video.c
+++ b/drivers/media/pci/tw686x/tw686x-video.c
@@ -1190,6 +1190,13 @@ int tw686x_video_init(struct tw686x_dev *dev)
return err;
}
 
+   /* Initialize vc->dev and vc->ch for the error path first */
+   for (ch = 0; ch < max_channels(dev); ch++) {
+   struct tw686x_video_channel *vc = >video_channels[ch];
+   vc->dev = dev;
+   vc->ch = ch;
+   }
+
for (ch = 0; ch < max_channels(dev); ch++) {
struct tw686x_video_channel *vc = >video_channels[ch];
struct video_device *vdev;
@@ -1198,9 +1205,6 @@ int tw686x_video_init(struct tw686x_dev *dev)
spin_lock_init(>qlock);
INIT_LIST_HEAD(>vidq_queued);
 
-   vc->dev = dev;
-   vc->ch = ch;
-
/* default settings */
err = tw686x_set_standard(vc, V4L2_STD_NTSC);
if (err)

Re: [PCIE device driver: tw686x] I have some problems with tw686x and I need help.

[Krzysztof Hałasa]

Hello Singh,

I've added linux-media as well as the current TW686x driver maintainer
to Cc: list. Perhaps they will be better prepared to help you, the
driver differs much from what it was when I originally wrote it.

 writes:

> First, I download source code from website
> https://github.com/torvalds/linux/tree/master/drivers/media/pci/tw686x,
> build the driver in my project. when i start the machine the machine
> printk the log
> "[ 5.557782] tw6869: PCI :01:00.0, IRQ 170, MMIO 0xc800 (memcpy mode)
> [ 5.565010] tw686x :01:00.0: enabling device ( -> 0002)".
> from the log we can see that pcie-bus read the ID is 6869, but out
> chip is tw6865. this is not our main problem.

The truth is I don't have a real TW6865, I simply assumed its PCI ID
would be 0x6865 because it worked this way for TW6864 and TW6869.
Could you please do a "lspci -vvn" on the machine with this card and
send me (with CC: linux-media@vger.kernel.org) this command's output
(you may omit other devices, I'm only interested in the TW chip
information). Perhaps the invalid identification can be fixed.

Thanks.

> After we booted the machine. then we lunch out application to display
> video that we capture from tw6865. But there are a lot of log , such
> as
> "[ 24.671551] tw686x :01:00.0: video10: unexpected p-b buffer!
> [ 24.671561] tw686x :01:00.0: video11: unexpected p-b buffer!
> [ 24.671566] tw686x :01:00.0: video12: unexpected p-b buffer!
> [ 24.671570] tw686x :01:00.0: video13: unexpected p-b buffer!"
> and the log print always.
> those log means tw6865 always reset dma channel.
> sometimes we can see the right picture, but the video moves slowly, about 2 
> frame per 5 seconds.
> after a few minute, the linux kernel crashed. I post the crash log
> below.

Unfortunately there is no meaningful stack dump. Perhaps the crashes
will go away when the "unexpected p-b buffer" issue is fixed so I
wouldn't worry too much about the dumps at the moment.

> Sometimes, after I booted the machine, and lunched the application. To
> the begin, we can see a few frame displaying on screen. after a few
> seconds, the monitor are freezed. and I use "cat /proc/interrupts" to
> look up tw6865's interrput. and we find that there is no tw6865
> interrupt any more. after a few minutes, most probely the machine will
> crash and reboot(crash log is same with the before which I have
> posted).
> we use command "dmesg" to look up log. we found that there ever
> happend "tw686x :01:00.0: video10: FIFO error" or "we found that
> there ever happend" frequently.

This could be caused by the invalid identification (and handling) of
the chip.

OTOH, the "memcpy" mode of this driver may be too slow on your machine
(ARM usually doesn't have hardware-coherent cache memory and certain
cache sync operations are apparently very slow). You may want to try the
DMA mode instead (modprobe tw686x dma_mode=contig or sg). Be aware that
the DMA modes aren't as flexible as memcpy mode, the frame formats are
more limited.

I don't have experience with any ARM64, though.

Also I think the current code will OOPS on any video initialization
error (which will most probably happen on ARM in contig mode because
ARM machines don't allow that much continuous allocation). At least this
can be easily fixed, I'll attach the patch.
-- 
Krzysztof Halasa

Industrial Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland

Re: [PATCH v2] media: solo6x10: fix lockup by avoiding delayed register write

[Krzysztof Hałasa]

Andrey Utkin  writes:

> --- a/drivers/media/pci/solo6x10/solo6x10.h
> +++ b/drivers/media/pci/solo6x10/solo6x10.h
> @@ -284,7 +284,10 @@ static inline u32 solo_reg_read(struct solo_dev 
> *solo_dev, int reg)
>  static inline void solo_reg_write(struct solo_dev *solo_dev, int reg,
> u32 data)
>  {
> + u16 val;
> +
>   writel(data, solo_dev->reg_base + reg);
> + pci_read_config_word(solo_dev->pdev, PCI_STATUS, );
>  }
>  
>  static inline void solo_irq_on(struct solo_dev *dev, u32 mask)

This is ok for now. I hope I will find some to refine this, so not all
register writes are done with the penalty - eventually.
-- 
Krzysztof Halasa

Industrial Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland
--
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: solo6010 modprobe lockup since e1ceb25a (v4.3 regression)

[Krzysztof Hałasa]

Andrey Utkin  writes:

> Lockup happens only on 6010. In provided log you can see that 6110
> passes just fine right before 6010. Also if 6010 PCI ID is removed from
> solo6x10 driver's devices list, the freeze doesn't happen.

Probably explains why I don't see lockups :-)

I will have a look.
-- 
Krzysztof Halasa

Industrial Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland
--
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: solo6010 modprobe lockup since e1ceb25a (v4.3 regression)

[Krzysztof Hałasa]

Andrey Utkin  writes:

>> Can you share some details about the machine you are experiencing the
>> problems on? CPU, chipset? I'd try to see if I can recreate the problem.
>
> See solo.txt.gz attached.

Thanks. I can see you have quite a set of video devices there.
I will see what I can do with this.

BTW does the lookup occur on SOLO6010, 6110, or both?
-- 
Krzysztof Halasa

Industrial Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland
--
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: solo6010 modprobe lockup since e1ceb25a (v4.3 regression)

[Krzysztof Hałasa]

Andrey Utkin  writes:

>> Does (only) adding the
>> 
>>  pci_read_config_word(solo_dev->pdev, PCI_STATUS, );
>> 
>> in solo_reg_write() help?
>
> Yes.
> I have posted a patch with this change few days ago, I thought you have
> noticed it.

Well, I think you haven't sent me a copy. Anyway, it would be great to
determine where exactly writes need a flush. Adding it everywhere is a
bit suboptimal, one would think.

Can you share some details about the machine you are experiencing the
problems on? CPU, chipset? I'd try to see if I can recreate the problem.

Alternatively, you could investigate yourself - at first you could put
pci_read_config_word() at the end of subroutines (including return
statements) using solo_reg_write(). And in that solo_p2m_dma_desc(),
before wait_for_completion_timeout(). Then eliminate them using some
sort of binary search to see which ones are required.
-- 
Krzysztof Halasa

Industrial Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland
--
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: solo6010 modprobe lockup since e1ceb25a (v4.3 regression)

[Krzysztof Hałasa]

Andrey Utkin <andrey_ut...@fastmail.com> writes:

> On Thu, Sep 22, 2016 at 10:51:37AM +0200, Krzysztof Hałasa wrote:
>> I wonder if the following fixes the problem (completely untested).
>
> I have given this a run, and it still hangs.

Does (only) adding the

pci_read_config_word(solo_dev->pdev, PCI_STATUS, );

in solo_reg_write() help?
-- 
Krzysztof Halasa

Industrial Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland
--
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: solo6010 modprobe lockup since e1ceb25a (v4.3 regression)

[Krzysztof Hałasa]

Andrey Utkin  writes:

> It happens in solo_disp_init at uploading default motion thresholds
> array.
>
> I've got a prints trace with solo6010-fix-lockup branch
> https://github.com/bluecherrydvr/linux/tree/solo6010-fix-lockup/drivers/media/pci/solo6x10
> the trace itself in jpg:
> https://decent.im:5281/upload/3793f393-e285-4514-83dd-bf08d1c8b4a2/e7ad898b-515b-4522-86a9-553daaeb0860.jpg

solo_motion_config() uses BM DMA and thus generates IRQ, this may be
indeed the ISR problem. BTW the IRQ debugging ("kernel hacking") should
catch it.
OTOH programming the DMA can be guilty as well.

I wonder if the following fixes the problem (completely untested).

diff --git a/drivers/media/pci/solo6x10/solo6x10-core.c 
b/drivers/media/pci/solo6x10/solo6x10-core.c
index f50d072..2d4900e 100644
--- a/drivers/media/pci/solo6x10/solo6x10-core.c
+++ b/drivers/media/pci/solo6x10/solo6x10-core.c
@@ -99,6 +99,7 @@ static irqreturn_t solo_isr(int irq, void *data)
 {
struct solo_dev *solo_dev = data;
u32 status;
+   u16 tmp;
int i;
 
status = solo_reg_read(solo_dev, SOLO_IRQ_STAT);
@@ -129,6 +130,7 @@ static irqreturn_t solo_isr(int irq, void *data)
if (status & SOLO_IRQ_G723)
solo_g723_isr(solo_dev);
 
+   pci_read_config_word(solo_dev->pdev, PCI_STATUS, ) // flush write 
to SOLO_IRQ_STAT
return IRQ_HANDLED;
 }
 
diff --git a/drivers/media/pci/solo6x10/solo6x10-p2m.c 
b/drivers/media/pci/solo6x10/solo6x10-p2m.c
index 07c4e07..8a51d45 100644
--- a/drivers/media/pci/solo6x10/solo6x10-p2m.c
+++ b/drivers/media/pci/solo6x10/solo6x10-p2m.c
@@ -70,6 +70,7 @@ int solo_p2m_dma_desc(struct solo_dev *solo_dev,
unsigned int config = 0;
int ret = 0;
int p2m_id = 0;
+   u16 tmp;
 
/* Get next ID. According to Softlogic, 6110 has problems on !=0 P2M */
if (solo_dev->type != SOLO_DEV_6110 && multi_p2m) {
@@ -111,6 +112,7 @@ int solo_p2m_dma_desc(struct solo_dev *solo_dev,
   desc[1].ctrl);
}
 
+   pci_read_config_word(solo_dev->pdev, PCI_STATUS, ); // flush writes
timeout = wait_for_completion_timeout(_dev->completion,
  solo_dev->p2m_jiffies);
 

> Indeed, targeted fixing would be more reasonable than making register
> r/w routines follow blocking fashion. But the driver is already complete
> and was known to be working, and I seems all places in code assume the
> blocking fashion of reg r/w, and changing that assumption may lead to
> covert bugs anywhere else, not just at probing, which may be hard to
> nail down.

The driver code doesn't have to assume anything about posted writes -
except at very specific places (as explained by Alan).

Normally, a CPU write to a register doesn't have to be flushed right
away. It would be much slower, especially if used extensively. Nobody
does anything alike since the end of the ISA bus.
The driver (and the card) can still see all operations in correct
order, in both cases.

The potential problem is a write being held in a buffer (and not making
it to the actual hardware). This may happen in ISR since the actual
write is deactivates the physical IRQ line. Otherwise the ISR terminates
and is immediately requested again - though this second call should
bring the IRQ down by reading the register (thus flushing the write
buffer) - so, while not very effective, it shouldn't lock up (but it's
a real bug worth fixing).

Also, I imagine a write to the DMA registers can be posted and the DMA
may not start in time. This shouldn't end in a lock up, either. Perhaps
a different bug is involved.


The other thing is BM DMA (card->RAM). All DMA transfers (initiated by
the card) are completed with an IRQ (either with success or failure).
This is potentially a problem as well, though it has nothing to do with
the patch in question. I guess the SOLO reads some descriptors or
something, and such writes are flushed this way.

> For now, I'll try setting pci_read_config_word() back instead of full
> revert. Does it need to be just in reg_write? No need for it in
> reg_read, right?

Sure, reg_read() doesn't write to the device.

It the patch doesn't fix the problem, what CPU and chipset are used by
the computer which exhibits the issue? Perhaps I have something similar
here and can reproduce it.
-- 
Krzysztof Halasa

Industrial Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland
--
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: solo6010 modprobe lockup since e1ceb25a (v4.3 regression)

[Krzysztof Hałasa]

Hans Verkuil  writes:

> That was probably the reason for the pci_read_config_word in the reg_write
> code. Try putting that back (and just that).

Yes. I guess a single pci_read_config_word() would suffice.

Though it would obviously be much better to identify the place in the
driver which needs to have the write buffers flushed, and add a read()
just there.

The interrupt handler maybe (e.g. just before the return IRQ_HANDLED)?

OTOH this may be some sort of timing problem, I mean the faster code may
put too much stress on the SOLO chip.

Doesn't happen here so I can't test the cure.
-- 
Krzysztof Halasa

Industrial Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland
--
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] [media] tw686x-kh: Delete an unnecessary check before the function call "video_unregister_device"

[Krzysztof Hałasa]

SF Markus Elfring  writes:

> The video_unregister_device() function tests whether its argument is NULL
> and then returns immediately. Thus the test around the call is not needed.
>
> This issue was detected by using the Coccinelle software.
>
> Signed-off-by: Markus Elfring 
> ---
>  drivers/staging/media/tw686x-kh/tw686x-kh-video.c | 3 +--

Thanks.
-- 
Krzysztof Halasa

Industrial Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland
--
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: Non-coherent (streaming) contig-dma?

[Krzysztof Hałasa]

Sakari Ailus  writes:

> I sent the set here under the subject "[RFC RESEND 00/11] vb2: Handle user
> cache hints, allow drivers to choose cache coherency" last September.

Thanks, I will look at this.
Unfortunately, I need CPU access to the buffers, it's just that coherent
RAM is way too slow on ARM.
-- 
Krzysztof Halasa

Industrial Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland
--
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

Non-coherent (streaming) contig-dma?

[Krzysztof Hałasa]

Hi,

I know certain approaches had been tried to allow use of streaming DMA
(dma_map_single() etc. - i.e., not coherent) in the media drivers, is
there something which can be used at this point (for MMAP method)?

Coherent buffers on many systems are very slow (uncacheable), should
i simply add/replace the necessary calls in dma-contig?

Any other options?

Is there any potential problem there?
-- 
Krzysztof Halasa

Industrial Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland
--
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] Driver for Intersil/Techwell TW686x-based PCIe frame grabbers again

[Krzysztof Hałasa]

Signed-off-by: Krzysztof Halasa <khal...@piap.pl>

diff --git a/MAINTAINERS b/MAINTAINERS
index f9c1a0f..c1759ce 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -11071,6 +11071,12 @@ W: https://linuxtv.org
 S: Odd Fixes
 F: drivers/media/pci/tw68/
 
+TW686x VIDEO4LINUX DRIVER
+M: Krzysztof Halasa <khal...@piap.pl>
+L: linux-media@vger.kernel.org
+S: Maintained
+F: drivers/staging/media/pci/tw686x/
+
 TPM DEVICE DRIVER
 M: Peter Huewe <peterhu...@gmx.de>
 M: Marcel Selhorst <tp...@selhorst.net>
diff --git a/drivers/staging/media/Kconfig b/drivers/staging/media/Kconfig
index 0078b6a..fbf6a66 100644
--- a/drivers/staging/media/Kconfig
+++ b/drivers/staging/media/Kconfig
@@ -37,6 +37,8 @@ source "drivers/staging/media/omap4iss/Kconfig"
 
 source "drivers/staging/media/timb/Kconfig"
 
+source "drivers/staging/media/tw686x/Kconfig"
+
 # Keep LIRC at the end, as it has sub-menus
 source "drivers/staging/media/lirc/Kconfig"
 
diff --git a/drivers/staging/media/Makefile b/drivers/staging/media/Makefile
index 9149588..22d4281 100644
--- a/drivers/staging/media/Makefile
+++ b/drivers/staging/media/Makefile
@@ -8,3 +8,4 @@ obj-$(CONFIG_VIDEO_OMAP1)   += omap1/
 obj-$(CONFIG_VIDEO_OMAP4)  += omap4iss/
 obj-$(CONFIG_DVB_MN88472)   += mn88472/
 obj-$(CONFIG_VIDEO_TIMBERDALE)  += timb/
+obj-$(CONFIG_VIDEO_TW686X) += tw686x/
diff --git a/drivers/staging/media/tw686x/Kconfig 
b/drivers/staging/media/tw686x/Kconfig
new file mode 100644
index 000..8792a68
--- /dev/null
+++ b/drivers/staging/media/tw686x/Kconfig
@@ -0,0 +1,16 @@
+config VIDEO_TW686X
+   tristate "Intersil/Techwell TW686x Video For Linux"
+   depends on VIDEO_DEV && PCI && VIDEO_V4L2
+   select VIDEOBUF2_DMA_SG
+   help
+ Support for Intersil/Techwell TW686x-based frame grabber cards.
+
+ Currently supported chips:
+ - TW6864 (4 video channels),
+ - TW6865 (4 video channels, not tested, second generation chip),
+ - TW6868 (8 video channels but only 4 first channels using
+   built-in video decoder are supported, not tested),
+ - TW6869 (8 video channels, second generation chip).
+
+ To compile this driver as a module, choose M here: the module
+ will be named tw686x.
diff --git a/drivers/staging/media/tw686x/Makefile 
b/drivers/staging/media/tw686x/Makefile
new file mode 100644
index 000..083b806
--- /dev/null
+++ b/drivers/staging/media/tw686x/Makefile
@@ -0,0 +1,3 @@
+tw686x-objs := tw686x-core.o tw686x-video.o
+
+obj-$(CONFIG_VIDEO_TW686X) += tw686x.o
diff --git a/drivers/staging/media/tw686x/TODO 
b/drivers/staging/media/tw686x/TODO
new file mode 100644
index 000..f226e80
--- /dev/null
+++ b/drivers/staging/media/tw686x/TODO
@@ -0,0 +1,3 @@
+TODO: implement V4L2_FIELD_INTERLACED* mode(s).
+
+Please Cc: patches to Krzysztof Halasa <khal...@piap.pl>.
diff --git a/drivers/staging/media/tw686x/tw686x-core.c 
b/drivers/staging/media/tw686x/tw686x-core.c
new file mode 100644
index 000..5891ea7
--- /dev/null
+++ b/drivers/staging/media/tw686x/tw686x-core.c
@@ -0,0 +1,140 @@
+/*
+ * Copyright (C) 2015 Industrial Research Institute for Automation
+ * and Measurements PIAP
+ *
+ * Written by Krzysztof Hałasa.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of version 2 of the GNU General Public License
+ * as published by the Free Software Foundation.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include "tw686x.h"
+#include "tw686x-regs.h"
+
+static irqreturn_t tw686x_irq(int irq, void *dev_id)
+{
+   struct tw686x_dev *dev = (struct tw686x_dev *)dev_id;
+   u32 int_status = reg_read(dev, INT_STATUS); /* cleared on read */
+   unsigned long flags;
+   unsigned handled = 0;
+
+   if (int_status) {
+   spin_lock_irqsave(>irq_lock, flags);
+   dev->dma_requests |= int_status;
+   spin_unlock_irqrestore(>irq_lock, flags);
+
+   if (int_status & 0xFFFF)
+   handled = tw686x_video_irq(dev);
+   }
+
+   return IRQ_RETVAL(handled);
+}
+
+static int tw686x_probe(struct pci_dev *pci_dev,
+   const struct pci_device_id *pci_id)
+{
+   struct tw686x_dev *dev;
+   int err;
+
+   dev = devm_kzalloc(_dev->dev, sizeof(*dev) +
+  (pci_id->driver_data & TYPE_MAX_CHANNELS) *
+  sizeof(dev->video_channels[0]), GFP_KERNEL);
+   if (!dev)
+   return -ENOMEM;
+
+   sprintf(dev->name, "TW%04X", pci_dev->device);
+   dev->type = pci_id->driver_data;
+
+   pr_info("%s: PCI %s, IRQ %d, MMIO 0x%lx\n", dev->name,
+   pci_name(pci_dev), pci_dev->irq,
+   (uns

Re: tw686x driver

[Krzysztof Hałasa]

Hans Verkuil  writes:

> Heck, if you prefer your driver can be added to staging first, then Ezequiel's
> driver commit can directly refer to the staging driver as being derived from 
> it.

Ok, I guess it's fair enough for me. Would you like me to send a patch
with paths changed to staging/?


However I'm not sure if Greg will like it - staging was meant for code
not otherwise ready for mainline. Not a place for alternate drivers.
-- 
Krzysztof Halasa

Industrial Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland
--
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: tw686x driver

[Krzysztof Hałasa]

Hans Verkuil  writes:

> Sorry, I meant V4L2_FIELD_INTERLACED support. Very few applications support
> FIELD_TOP/BOTTOM, let alone SEQ_BT.

Well, that's doable, though not in SG mode. It still doesn't require
memcpy() of uncompressed video.

> I don't get it. Getting your driver in staging is much better for you since
> your code is in there and can be compiled for those who want to. I'm not
> going to add your driver and then replace it with Ezequiel's version.

Then simply add my driver and don't replace it.
Face it: Ezequiel's driver adds the audio support, and I guess he can
add this audio code without breaking the existing driver.
I also have (old) audio code for this driver, but it has only been
tested without an actual audio input, so it's not ready for deployment.
I simply don't use audio at the moment.

Otherwise, "his driver" is a regression - it removes critical
functionality, in exchange giving only the V4L2_FIELD_INTERLACED, which
can be easily implemented without breaking the rest.

> Heck, if you prefer your driver can be added to staging first, then Ezequiel's
> driver commit can directly refer to the staging driver as being
> derived from it.

Well, I will have to think about it. Though I wonder - if you do that,
perhaps my next request should be to swap them.
-- 
Krzysztof Halasa

Industrial Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland
--
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: tw686x driver

[Krzysztof Hałasa]

Hans Verkuil  writes:

> I have two drivers with different feature sets. Only one can be active
> at a time. I have to make a choice which one I'll take and Ezequiel's
> version has functionality (audio, interlaced support) which matches best
> with existing v4l applications and the typical use cases. I'm not going
> to have two drivers for the same hw in the media subsystem since only
> one can be active anyway. My decision, although Mauro can of course decide
> otherwise.

(BTW my driver supports interlace)

> I am OK with adding your driver to staging in the hope that someone will
> merged the functionalities of the two to make a new and better driver.

Then I don't really understand why there can be two drivers for the same
hw in the tree, but one has to be in "staging".
Staging isn't meant for this. My driver perfectly qualifies for being
merged in the non-staging media directory - doesn't it?

You are right, there can be two drivers in the tree for the same hw,
examples are known. You don't have to make a choice here, though you are
free to do so.

> My goal is to provide the end-user with the best experience, and this is
> IMHO the best option given the hand I've been dealt.

Then, if the moral side of the story can't be maintained, at least do it
legally as required by copyright laws (and the GPL license as well).
Doing so is not a "pollution" of git history, but your responsibility as
a maintainer.


To be honest, I still can't understand why are you afraid of adding
Ezequiel's changes on top of my driver properly. While probably far from
being a pretty changeset, it would make it legal, and this is the thing
that the author, I suppose, is entitled to.
Adding some "link" to a mail archive(?) is not a substitute.
-- 
Krzysztof Halasa

Industrial Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland
--
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: tw686x driver

[Krzysztof Hałasa]

Hans Verkuil  writes:

>> Staging is meant for completely different situation - for immature,
>> incomplete code. It has nothing to do with the case.
>
> It can be for anything that prevents it from being mainlined. It was (still 
> is?)
> used for mature android drivers, for example.

What is preventing my driver from being mainlined?
-- 
Krzysztof Halasa

Industrial Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland
--
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: tw686x driver

[Krzysztof Hałasa]

Hans Verkuil  writes:

> There is no point whatsoever in committing a driver and then replacing it
> with another which has a different feature set. I'm not going to do
> that.

Sure, that's why I haven't asked you to do it.
Now there is no another driver, as Ezequiel stated - there is just one
driver.

The point is clear, showing who exactly wrote what.

> One option that might be much more interesting is to add your driver to
> staging with a TODO stating that the field support should be added to
> the mainline driver.

Field mode is one thing. What's a bit more important is that Ezequiel's
changes take away the SG DMA, and basically all DMA. The chip has to use
DMA, but his driver then simply memcpy() all the data to userspace
buffers. This doesn't work on low-power machines.

Staging is meant for completely different situation - for immature,
incomplete code. It has nothing to do with the case.

> I'm not sure if Mauro would go for it, but I think this is a fair option.

I don't expect the situation to be fair to me, anymore.

I also don't want to pursue the legal stuff, copyright laws etc.,
but a quick glance at the COPYING file at the root of the kernel sources
may be helpful:

> 2. You may modify your copy or copies of the Program or any portion
> of it, thus forming a work based on the Program, and copy and
> distribute such modifications or work under the terms of Section 1
> above, provided that you also meet all of these conditions:
>
> a) You must cause the modified files to carry prominent notices
> stating that you changed the files and the date of any change.

I don't even ask for that much - I only ask that the single set of
changes from Ezequiel has this very information. This is BTW one of the
reasons we switched to git.
-- 
Krzysztof Halasa

Industrial Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland
--
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: tw686x driver

[Krzysztof Hałasa]

Hans Verkuil  writes:

> When a driver is merged for the first time in the kernel it is always as
> a single change, i.e. you don't include the development history as that
> would pollute the kernel's history.

We don't generally add new drivers with long patch series, that's right.
That's because there is usually no reason to do so. This situation is
different, there is a very good reason.

I'm not asking for doing this with a long patch set. A single patch from
me + single patch containing the Ezequiel changes would suffice.

> Those earlier versions have never
> been tested/reviewed to the same extent as the final version

This is not a real problem, given the code will be changed immediately
again.

> and adding
> them could break git bisect.

Not really. You can apply the version based on current media tree,
I have posted it a week ago or so. This doesn't require any effort.

BTW applying the thing in two consecutive patches (the old version +
changes) doesn't break git bisect at all. It only breaks the compiling,
and only in the very case when git bisect happens to stop between the
first and second patch, and the driver is configured in. Very unlikely,
though the remedy is very easy as shown in "man git-bisect".
This is a common thing when you do git bisect, though the related
patches are usually much more distant and thus the probability that the
kernel won't compile is much much greater.

Alternatively one could apply the original version to an older kernel and
merge the product. Less trivial and I don't know why one would want to
do so, given #1.

One could also disable the CONFIG_VIDEO_TW686X in Kconfig (at the
original patch). Possibilities are endless.

We have to face it: there is absolutely no problem with adding the
driver with two patches, either using my original patch or the updated
one. And it doesn't require any effort, just a couple of git commands.

> It is a quite unusual situation and the only way I could make it worse would
> by not merging anything.

Not really.
There is a very easy way out. You can just add the driver properly with
two patches.


Though I don't know why do you think my driver alone doesn't qualify to
be merged (without subsequent Ezequiel's changes). It's functional
and stable, and I have been using it (in various versions) for many
months. It's much more mature than many other drivers which make it to
the kernel regularly.

It is definitely _not_ my driver that is problematic.
-- 
Krzysztof Halasa

Industrial Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland
--
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: tw686x driver

[Krzysztof Hałasa]

Hi Hans,

Hans Verkuil  writes:

> So lessons learned:
>
> Krzysztof, next time don't wait many months before posting a new version 
> fixing
> requested changes.

Actually, this is not how it happened.

On July 3, 2015 I posted the original driver:
http://www.spinics.net/lists/linux-media/msg91474.html

Ezequiel reviewed the code on 6 Jul 2015:
http://www.spinics.net/lists/linux-media/msg91516.html

On 27 Jul 2015, as you can easily find in your own mail archive, he
informed me that he's working on the driver and that he's going to post
updated patches himself. This was holidays time for me and I stated
that I have to suspend my work till the end of August.

I asked him to add his changes on top of my code several times (and this
is really easy with git). This never happened, and on 14 Aug 2015 he
posted:

> Problem is I've re-written the driver, taking yours as a starting point
> and reference.

> In other words, I don't have a proper git branch with a history, starting
> from the patch you submitted. Instead, I would be submitting a new
> patch for Hans and Mauro to review.

Maybe I got the meaning of this wrong, and he wasn't writing about
rewriting the driver "from scratch". Yes, after receiving this mail
I stopped my development, waiting for his driver to show up. That's
where the "many months" are. Yes, Ezequiel waited for "many months" with
his version - not me.

Only after he has posted "his" driver, I could find out what the
"rewriting" meant.

Don't get me wrong, I was never opposed to him improving my driver.
I only requested that his contributions are clearly shown, in a form
of a patch or a patch set (or a git tree etc.), and so are my own.
I really can't understand why his code can't be transparently applied
over my original patch (or the updated one, which compiles and works
fine with recent media tree).

Is it too much to ask?


The lesson I learned is thus this instead:
- don't publish code because it can be hijacked, twisted and you'll
have to fight for even getting your authorship back. Forget about proper
attribution and history.
-- 
Krzysztof Halasa

Industrial Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland
--
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: H264 headers generation for driver

[Krzysztof Hałasa]

Andrey Utkin  writes:

[H.264 headers]
> I guess that one acceptable way is to pre-generate all headers for all
> needed cases and ship them inlined; for correctness checking purpose,
> it is possible to ship also a script or additional source code file
> which is able to generate same headers.

I think these are good ideas.
BTW the SOLO6110 at the moment also has pregenerated SPS/PPS headers
(whole NALs actually).
-- 
Krzysztof Halasa

Industrial Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland
--
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: TW686x

[Krzysztof Hałasa]

Hi Ezequiel,

OTOH I don't see any reason preventing you from sending a pull request
for the inclusion of the TW686x driver, Mauro could merge this stuff
then (assuming it's ready).

You don't have to wait for me and a driver doesn't need to be a single
patch from a single person.
-- 
Krzysztof Hałasa

Przemysłowy Instytut Automatyki i Pomiarów PIAP
Al. Jerozolimskie 202, 02-486 Warszawa
--
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: Subjective maturity of tw6869, cx25821, bluecherry/softlogic drivers

[Krzysztof Hałasa]

Andrey Utkin andrey.ut...@corp.bluecherry.net writes:

 Also, TW686x are (mini) PCIe while SOLO6110 (and earlier SOLO6010 which
 produces MPEG4 part 2 instead of H.264) are (mini) PCI.

 solo6110 is PCI-E, not PCI.

No, it's not, both SOLO6010 and SOLO6110 are 32-bit PCI.

SOLO6110 Data Sheet
1.2.5. PCI/HOST Interface
 - PCI Local Bus Specification, Rev. 2.2. 32bit/33MHz(66MHz)
 - PCI Master/Target Mode.
 - 32bit CPU Host Interface

There are probably PCIe cards using SOLO6110, but they have to use
a PCIe-PCI bridge.

One can also use a SOLO6x10 card with a separate converter board.
We're using converters with PLX PEX8112 bridge chip for this.
-- 
Krzysztof Halasa

Industrial Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] [MEDIA] Add support for TW686[4589]-based frame grabbers.

[Krzysztof Hałasa]

Audio will be supported with a further patch.

Signed-off-by: Krzysztof Hałasa khal...@piap.pl

diff --git a/drivers/media/pci/Kconfig b/drivers/media/pci/Kconfig
index 218144a..32902f2 100644
--- a/drivers/media/pci/Kconfig
+++ b/drivers/media/pci/Kconfig
@@ -21,6 +21,7 @@ source drivers/media/pci/zoran/Kconfig
 source drivers/media/pci/saa7146/Kconfig
 source drivers/media/pci/solo6x10/Kconfig
 source drivers/media/pci/tw68/Kconfig
+source drivers/media/pci/tw686x/Kconfig
 endif
 
 if MEDIA_ANALOG_TV_SUPPORT || MEDIA_DIGITAL_TV_SUPPORT
diff --git a/drivers/media/pci/Makefile b/drivers/media/pci/Makefile
index 0baf0d2..5b15aa6 100644
--- a/drivers/media/pci/Makefile
+++ b/drivers/media/pci/Makefile
@@ -24,6 +24,7 @@ obj-$(CONFIG_VIDEO_BT848) += bt8xx/
 obj-$(CONFIG_VIDEO_SAA7134) += saa7134/
 obj-$(CONFIG_VIDEO_SAA7164) += saa7164/
 obj-$(CONFIG_VIDEO_TW68) += tw68/
+obj-$(CONFIG_VIDEO_TW686X) += tw686x/
 obj-$(CONFIG_VIDEO_MEYE) += meye/
 obj-$(CONFIG_STA2X11_VIP) += sta2x11/
 obj-$(CONFIG_VIDEO_SOLO6X10) += solo6x10/
diff --git a/drivers/media/pci/tw686x/Kconfig b/drivers/media/pci/tw686x/Kconfig
new file mode 100644
index 000..8792a68
--- /dev/null
+++ b/drivers/media/pci/tw686x/Kconfig
@@ -0,0 +1,16 @@
+config VIDEO_TW686X
+   tristate Intersil/Techwell TW686x Video For Linux
+   depends on VIDEO_DEV  PCI  VIDEO_V4L2
+   select VIDEOBUF2_DMA_SG
+   help
+ Support for Intersil/Techwell TW686x-based frame grabber cards.
+
+ Currently supported chips:
+ - TW6864 (4 video channels),
+ - TW6865 (4 video channels, not tested, second generation chip),
+ - TW6868 (8 video channels but only 4 first channels using
+   built-in video decoder are supported, not tested),
+ - TW6869 (8 video channels, second generation chip).
+
+ To compile this driver as a module, choose M here: the module
+ will be named tw686x.
diff --git a/drivers/media/pci/tw686x/Makefile 
b/drivers/media/pci/tw686x/Makefile
new file mode 100644
index 000..083b806
--- /dev/null
+++ b/drivers/media/pci/tw686x/Makefile
@@ -0,0 +1,3 @@
+tw686x-objs := tw686x-core.o tw686x-video.o
+
+obj-$(CONFIG_VIDEO_TW686X) += tw686x.o
diff --git a/drivers/media/pci/tw686x/tw686x-core.c 
b/drivers/media/pci/tw686x/tw686x-core.c
new file mode 100644
index 000..aa873c5
--- /dev/null
+++ b/drivers/media/pci/tw686x/tw686x-core.c
@@ -0,0 +1,176 @@
+/*
+  Copyright (C) 2015 Industrial Research Institute for Automation
+  and Measurements PIAP
+
+  Written by Krzysztof Hałasa.
+
+  This program is free software; you can redistribute it and/or modify it
+  under the terms of version 2 of the GNU General Public License
+  as published by the Free Software Foundation.
+*/
+
+#include linux/init.h
+#include linux/interrupt.h
+#include linux/kernel.h
+#include linux/module.h
+#include linux/slab.h
+#include tw686x.h
+#include tw686x-regs.h
+
+static irqreturn_t tw686x_irq(int irq, void *dev_id)
+{
+   struct tw686x_dev *dev = (struct tw686x_dev *)dev_id;
+   u32 int_status = reg_read(dev, INT_STATUS); /* cleared on read */
+   unsigned long flags;
+   unsigned handled = 0;
+
+   if (int_status) {
+   spin_lock_irqsave(dev-irq_lock, flags);
+   dev-dma_requests |= int_status;
+   spin_unlock_irqrestore(dev-irq_lock, flags);
+
+   if (int_status  0xFFFF)
+   handled = tw686x_video_irq(dev);
+   }
+
+   return IRQ_RETVAL(handled);
+}
+
+static int tw686x_probe(struct pci_dev *pci_dev,
+   const struct pci_device_id *pci_id)
+{
+   struct tw686x_dev *dev;
+   int err;
+
+   dev = kzalloc(sizeof(*dev) + (pci_id-driver_data  TYPE_MAX_CHANNELS) *
+ sizeof(dev-video_channels[0]), GFP_KERNEL);
+   if (!dev)
+   return -ENOMEM;
+
+   sprintf(dev-name, TW%04X, pci_dev-device);
+   dev-type = pci_id-driver_data;
+
+   pr_info(%s: PCI %s, IRQ %d, MMIO 0x%lx\n, dev-name,
+   pci_name(pci_dev), pci_dev-irq,
+   (unsigned long)pci_resource_start(pci_dev, 0));
+
+   dev-pci_dev = pci_dev;
+   if (pci_enable_device(pci_dev)) {
+   err = -EIO;
+   goto free_dev;
+   }
+
+   pci_set_master(pci_dev);
+
+   if (!pci_dma_supported(pci_dev, DMA_BIT_MASK(32))) {
+   pr_err(%s: 32-bit PCI DMA not supported\n, dev-name);
+   err = -EIO;
+   goto disable;
+   }
+
+   if (!request_mem_region(pci_resource_start(pci_dev, 0),
+   pci_resource_len(pci_dev, 0), dev-name)) {
+   pr_err(%s: Unable to get MMIO region\n, dev-name);
+   err = -EBUSY;
+   goto disable;
+   }
+
+   dev-mmio = ioremap_nocache(pci_resource_start(pci_dev, 0),
+   pci_resource_len(pci_dev, 0));
+   if (!dev-mmio) {
+   pr_err(%s

Re: Subjective maturity of tw6869, cx25821, bluecherry/softlogic drivers

[Krzysztof Hałasa]

Nathaniel Bezanson mys...@telcodata.us writes:

 I found the intersil/techwell TW6869 chip on a very affordable card,
 and there's a nice looking driver here:
 https://github.com/igorizyumin/tw6869/

It didn't work for me. Probably because it uses large coherent
allocations (which aren't possible on ARM, and shouldn't be used
on any hardware). Other problems (not sure if in this very driver, or
another of its clones): not using DMA SG, using CPU to copy frame data
in DQBUF (or somewhere), apparent support for nonexistent hardware
features (tuners?).

This reminds me to post my own driver for these cards, a bit smaller and
lacking some features (like scaling and cropping) but working and using
SG DMA.

 Only trouble is there only
 seems to be the one card using it

There are at least Sensoray (8-port, a bit improved chip) and Commell
(4-port, based on older TW6864) cards.

These chips aren't ideal - but they work.
For example, in DMA SG mode, they can't produce a single continuous
interlaced frame in memory - they can only make two separate fields.
A limitation of their DMA engine, since in non-SG mode they can make
(pseudo)-progressive frames.

Also, they can't do YUV420 - they only do things like YUV422 and
RGB565/555 (well they can do YUV420 with custom encoding in a mode with
reduced vertical resolution - with field dropping or in field mode,
where each field is a different picture encoded with YUV420).


I'm using SOLO6110-based cards, too. They work. There are apparently
certain problems, but I wasn't able to reproduce them. My use is limited
to 4-channel operation, though (the chip can do up to 5 full D1 H.264
streams, but there are 16-channel versions which can do 16 simultaneous
H.264s with a reduced resolution and/or with frame dropping - I suspect
the latter could be the problem).

SOLO6110 doesn't produce a valid interlaced H.264 stream (with field
encoding) - only a (pseudo) progressive stream (frame mode). This is
actually what TW686x lacks :-)

Also, TW686x are (mini) PCIe while SOLO6110 (and earlier SOLO6010 which
produces MPEG4 part 2 instead of H.264) are (mini) PCI.
TW686x don't have hardware H.264 encoder (though I'm told certain
TW586x do have).
-- 
Krzysztof Halasa

Industrial Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland

--
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

Video driver for Techwell TW686[4589]-based cards.

[Krzysztof Hałasa]

I'll be attaching a driver for Techwell/Intersil TW686[4589]-based video
frame grabbers. It's currently tested only with v4.0 (the system I'm
using this on has problems with v4.1) but it should apply and work with
current kernels (there might be a trivial conflict in Kconfig and/or
Makefile).

Fire away.
-- 
Krzysztof Halasa

Industrial Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] MEDIA PCI Kconfig ANALOG_TV - CAMERA

[Krzysztof Hałasa]

Hi,

I noticed certain cards are currently under MEDIA_ANALOG_TV_SUPPORT
but it seems they are frame grabbers (with CVBS, Svideo etc. inputs)
rather than TV receivers (with analog TV tuners).

MEDIA_CAMERA_SUPPORT maybe isn't the best name (only meye driver seems
to drive a real camera in a laptop) but it at least doesn't select the
TUNERs.

Perhaps the following patch would make sense.

Signed-off-by: Krzysztof Hałasa khal...@piap.pl

--- a/drivers/media/pci/Kconfig
+++ b/drivers/media/pci/Kconfig
@@ -11,16 +11,16 @@ if MEDIA_PCI_SUPPORT
 if MEDIA_CAMERA_SUPPORT
comment Media capture support
 source drivers/media/pci/meye/Kconfig
+source drivers/media/pci/solo6x10/Kconfig
 source drivers/media/pci/sta2x11/Kconfig
+source drivers/media/pci/tw68/Kconfig
+source drivers/media/pci/zoran/Kconfig
 endif
 
 if MEDIA_ANALOG_TV_SUPPORT
comment Media capture/analog TV support
 source drivers/media/pci/ivtv/Kconfig
-source drivers/media/pci/zoran/Kconfig
 source drivers/media/pci/saa7146/Kconfig
-source drivers/media/pci/solo6x10/Kconfig
-source drivers/media/pci/tw68/Kconfig
 endif
 
 if MEDIA_ANALOG_TV_SUPPORT || MEDIA_DIGITAL_TV_SUPPORT
-- 
Krzysztof Halasa

Industrial Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland
--
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] SOLO6x10: Fix G.723 minimum audio period count.

[Krzysztof Hałasa]

The period count is fixed, don't confuse ALSA.

Signed-off-by: Krzysztof Hałasa khal...@piap.pl

--- a/drivers/media/pci/solo6x10/solo6x10-g723.c
+++ b/drivers/media/pci/solo6x10/solo6x10-g723.c
@@ -48,10 +48,8 @@
 /* The solo writes to 1k byte pages, 32 pages, in the dma. Each 1k page
  * is broken down to 20 * 48 byte regions (one for each channel possible)
  * with the rest of the page being dummy data. */
-#define G723_MAX_BUFFER(G723_PERIOD_BYTES * PERIODS_MAX)
+#define PERIODSG723_FDMA_PAGES
 #define G723_INTR_ORDER4 /* 0 - 4 */
-#define PERIODS_MIN(1  G723_INTR_ORDER)
-#define PERIODS_MAXG723_FDMA_PAGES
 
 struct solo_snd_pcm {
int on;
@@ -130,11 +128,11 @@ static const struct snd_pcm_hardware snd_solo_pcm_hw = {
.rate_max   = SAMPLERATE,
.channels_min   = 1,
.channels_max   = 1,
-   .buffer_bytes_max   = G723_MAX_BUFFER,
+   .buffer_bytes_max   = G723_PERIOD_BYTES * PERIODS,
.period_bytes_min   = G723_PERIOD_BYTES,
.period_bytes_max   = G723_PERIOD_BYTES,
-   .periods_min= PERIODS_MIN,
-   .periods_max= PERIODS_MAX,
+   .periods_min= PERIODS,
+   .periods_max= PERIODS,
 };
 
 static int snd_solo_pcm_open(struct snd_pcm_substream *ss)
@@ -340,7 +338,8 @@ static int solo_snd_pcm_init(struct solo_dev *solo_dev)
ret = snd_pcm_lib_preallocate_pages_for_all(pcm,
SNDRV_DMA_TYPE_CONTINUOUS,
snd_dma_continuous_data(GFP_KERNEL),
-   G723_MAX_BUFFER, G723_MAX_BUFFER);
+   G723_PERIOD_BYTES * PERIODS,
+   G723_PERIOD_BYTES * PERIODS);
if (ret  0)
return ret;
 
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] SOLO6x10: remove unneeded register locking and barriers.

[Krzysztof Hałasa]

readl() and writel() are atomic, we don't need the spin lock.
Also, flushing posted write buffer isn't required. Especially on read :-)

Signed-off-by: Krzysztof Hałasa khal...@piap.pl

--- a/drivers/media/pci/solo6x10/solo6x10-core.c
+++ b/drivers/media/pci/solo6x10/solo6x10-core.c
@@ -483,7 +483,6 @@ static int solo_pci_probe(struct pci_dev *pdev, const 
struct pci_device_id *id)
 
solo_dev-type = id-driver_data;
solo_dev-pdev = pdev;
-   spin_lock_init(solo_dev-reg_io_lock);
ret = v4l2_device_register(pdev-dev, solo_dev-v4l2_dev);
if (ret)
goto fail_probe;
--- a/drivers/media/pci/solo6x10/solo6x10.h
+++ b/drivers/media/pci/solo6x10/solo6x10.h
@@ -201,7 +201,6 @@ struct solo_dev {
int nr_ext;
u32 irq_mask;
u32 motion_mask;
-   spinlock_t  reg_io_lock;
struct v4l2_device  v4l2_dev;
 
/* tw28xx accounting */
@@ -283,36 +282,13 @@ struct solo_dev {
 
 static inline u32 solo_reg_read(struct solo_dev *solo_dev, int reg)
 {
-   unsigned long flags;
-   u32 ret;
-   u16 val;
-
-   spin_lock_irqsave(solo_dev-reg_io_lock, flags);
-
-   ret = readl(solo_dev-reg_base + reg);
-   rmb();
-   pci_read_config_word(solo_dev-pdev, PCI_STATUS, val);
-   rmb();
-
-   spin_unlock_irqrestore(solo_dev-reg_io_lock, flags);
-
-   return ret;
+   return readl(solo_dev-reg_base + reg);
 }
 
 static inline void solo_reg_write(struct solo_dev *solo_dev, int reg,
  u32 data)
 {
-   unsigned long flags;
-   u16 val;
-
-   spin_lock_irqsave(solo_dev-reg_io_lock, flags);
-
writel(data, solo_dev-reg_base + reg);
-   wmb();
-   pci_read_config_word(solo_dev-pdev, PCI_STATUS, val);
-   rmb();
-
-   spin_unlock_irqrestore(solo_dev-reg_io_lock, flags);
 }
 
 static inline void solo_irq_on(struct solo_dev *dev, u32 mask)
--
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] SOLO6x10: Remove dead code.

[Krzysztof Hałasa]

solo_dev and pdev cannot be NULL here. It doesn't matter if we
initialized the PCI device or not.

Signed-off-by: Krzysztof Hałasa khal...@piap.pl

--- a/drivers/media/pci/solo6x10/solo6x10-core.c
+++ b/drivers/media/pci/solo6x10/solo6x10-core.c
@@ -134,23 +134,11 @@ static irqreturn_t solo_isr(int irq, void *data)
 
 static void free_solo_dev(struct solo_dev *solo_dev)
 {
-   struct pci_dev *pdev;
-
-   if (!solo_dev)
-   return;
+   struct pci_dev *pdev = solo_dev-pdev;
 
if (solo_dev-dev.parent)
device_unregister(solo_dev-dev);
 
-   pdev = solo_dev-pdev;
-
-   /* If we never initialized the PCI device, then nothing else
-* below here needs cleanup */
-   if (!pdev) {
-   kfree(solo_dev);
-   return;
-   }
-
if (solo_dev-reg_base) {
/* Bring down the sub-devices first */
solo_g723_exit(solo_dev);
@@ -164,8 +152,7 @@ static void free_solo_dev(struct solo_dev *solo_dev)
 
/* Now cleanup the PCI device */
solo_irq_off(solo_dev, ~0);
-   if (pdev-irq)
-   free_irq(pdev-irq, solo_dev);
+   free_irq(pdev-irq, solo_dev);
pci_iounmap(pdev, solo_dev-reg_base);
}
 
--
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] SOLO6x10: unmap registers only after free_irq().

[Krzysztof Hałasa]

Fixes a panic on ARM. Diagnosis by Russell King.

Signed-off-by: Krzysztof Hałasa khal...@piap.pl

--- a/drivers/media/pci/solo6x10/solo6x10-core.c
+++ b/drivers/media/pci/solo6x10/solo6x10-core.c
@@ -164,9 +164,9 @@ static void free_solo_dev(struct solo_dev *solo_dev)
 
/* Now cleanup the PCI device */
solo_irq_off(solo_dev, ~0);
-   pci_iounmap(pdev, solo_dev-reg_base);
if (pdev-irq)
free_irq(pdev-irq, solo_dev);
+   pci_iounmap(pdev, solo_dev-reg_base);
}
 
pci_release_regions(pdev);
--
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

A few SOLO6x10 patches.

[Krzysztof Hałasa]

Hi,

I'm attaching a few patches to SOLO6x10 video frame grabber driver.
Nothing out of ordinary, an audio bug fix (nobody using SOLO with
audio?), an rmmod-related panic and the register access optimization.
Feel free to pick up what you want.

The remaining issue is incorrect SDRAM size reported in certain cases.
I still have to investigate it a bit.
-- 
Krzysztof Halasa

Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland
--
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: On register r/w macros/procedures of drivers/media/pci

[Krzysztof Hałasa]

Andrey Utkin andrey.ut...@corp.bluecherry.net writes:

 Please check first digit. I mean _5_864, in your post there's 6869.

Ok, I just thought it may be a typo. I can now see 5864 is a H.264
encoder, while 686x are simpler frame-grabbers only.

Sorry for the noise.
-- 
Krzysztof Halasa

Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland
--
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: On register r/w macros/procedures of drivers/media/pci

[Krzysztof Hałasa]

Andrey Utkin andrey.ut...@corp.bluecherry.net writes:

 I am starting a work on driver for techwell tw5864 media grabberencoder.

If this is tw6864 then I have a driver mostly completed.
Actually I'm using tw6869 but I think this is very similar (4 channels
instead of 8 and PCI instead of PCIe). I have 6864s but haven't yet
tried using them for trivial reasons.

This is BTW completely different chip (family) than the old tw68x.
-- 
Krzysztof Halasa

Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland
--
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] solo6x10 freeze, even with Oct 31's linux-next... any ideas or help?

[Krzysztof Hałasa]

Andrey Utkin andrey.ut...@corp.bluecherry.net writes:

 In upstream there's no more module parameter for video standard
 (NTSC/PAL). But there's VIDIOC_S_STD handling procedure. But it turns
 out not to work correctly: the frame is offset, so that in the bottom
 there's black horizontal bar.
 The S_STD ioctl call actually makes difference, because without that
 the frame slides vertically all the time. But after the call the
 picture is not correct.

Which kernel version are you using?
I remember there were some problems with earlier versions, where the
NTSC vs PAL wasn't consistenly a bool but rather a raw register value
(or something like this), but it was fixed last time I checked.
I'm personally using SOLO6110-based cards with v3.17 and PAL and it
works, with minimal unrelated patches.

 Any ideas why wouldn't it work to change the mode after the driver load?
 Would it be allowed to add back that kernel module parameter (the one
 passed at module load time)?

I don't think this alone would help.

Looking at my patch queue (will try to remember to have them posted)...
Well, it could be the SDRAM size detection routine. I'm using cards with
64 MB of RAM and the routine repeatedly detected 128 MB or so (max
supported). I have a temporary fix for this but it needs a bit more
work, I have seen a case when it failed (I'm using ARM and MIPS
platforms and they may differ from x86 in endianness, cache coherency
etc).

If you have a card with 64 MB RAM you may want to check if the driver
detects it correctly, and if not e.g. hardcode the size. Otherwise,
I have no idea what could be wrong, it works for me.
-- 
Krzysztof Halasa

Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland
--
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] solo6x10 freeze, even with Oct 31's linux-next... any ideas or help?

[Krzysztof Hałasa]

Andrey Utkin andrey.ut...@corp.bluecherry.net writes:

 The problem is the following: after ~1 hour of uptime with working
 application reading the streams, one card (the same one every time)
 stops producing interrupts (counter in /proc/interrupts freezes), and
 all threads reading from that card hang forever in
 ioctl(VIDIOC_DQBUF).

There is a race condition in the IRQ handler, at least in 3.17.
I don't know if it's related, will post a patch.
-- 
Krzysztof Halasa

Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland
--
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

SOLO6x10: fix a race in IRQ handler.

[Krzysztof Hałasa]

The IRQs have to be acknowledged before they are serviced, otherwise some events
may be skipped. Also, acknowledging IRQs just before returning from the handler
doesn't leave enough time for the device to deassert the INTx line, and for
bridges to propagate this change. This resulted in twice the IRQ rate on ARMv6
dual core CPU.

Signed-off-by: Krzysztof Hałasa khal...@piap.pl

--- a/drivers/media/pci/solo6x10/solo6x10-core.c
+++ b/drivers/media/pci/solo6x10/solo6x10-core.c
@@ -105,11 +105,8 @@ static irqreturn_t solo_isr(int irq, void *data)
if (!status)
return IRQ_NONE;
 
-   if (status  ~solo_dev-irq_mask) {
-   solo_reg_write(solo_dev, SOLO_IRQ_STAT,
-  status  ~solo_dev-irq_mask);
-   status = solo_dev-irq_mask;
-   }
+   /* Acknowledge all interrupts immediately */
+   solo_reg_write(solo_dev, SOLO_IRQ_STAT, status);
 
if (status  SOLO_IRQ_PCI_ERR)
solo_p2m_error_isr(solo_dev);
@@ -132,9 +129,6 @@ static irqreturn_t solo_isr(int irq, void *data)
if (status  SOLO_IRQ_G723)
solo_g723_isr(solo_dev);
 
-   /* Clear all interrupts handled */
-   solo_reg_write(solo_dev, SOLO_IRQ_STAT, status);
-
return IRQ_HANDLED;
 }
 

-- 
Krzysztof Halasa

Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland
--
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: SOLO6x10: fix a race in IRQ handler.

[Krzysztof Hałasa]

Andrey Utkin andrey.krieger.ut...@gmail.com writes:

 could you please point to some reading which explains this moment? Or
 you just know this from experience? The solo device specs are very
 terse about this, so I considered that it should work fine without
 regard to how fast we write back to that register.

The SOLO IRQ controller does the common thing, all drivers (for chips
using the relatively modern write 1 to clear) have to follow this
sequence: first ACK the interrupts sources (so they are deasserted,
though they can be asserted again if new events arrive), and only then
service the chip.

 Also while you're at it, and if this really makes sense, you could
 merge these two writes (unrecognized bits, then recognized bits) to
 one write act.

I think my patch does exactly this, merges both writes.
-- 
Krzysztof Halasa

Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland
--
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: Suspected cache coherency problem on V4L2 and AR7100 CPU

[Krzysztof Hałasa]

Ralf Baechle r...@linux-mips.org writes:

 16K is a silver bullet solution to all cache aliasing problems.  So if
 your issue persists with 16K page size, it's not a cache aliasing issue.
 Aside there are generally performance gains from the bigger page size.

I wonder why isn't the issue present in other cases. Perhaps remapping
of a userspace address and accessing it with kseg0 isn't a frequent
operation.

Shouldn't we change the default page size (on affected CPUs) to 16 KB
then? Alternatively, we could flush/invalidate the cache when needed -
is it a viable option?

Thanks.
-- 
Krzysztof Halasa

Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland
--
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: Suspected cache coherency problem on V4L2 and AR7100 CPU

[Krzysztof Hałasa]

Ralf Baechle r...@linux-mips.org writes:

 The kernel is supposed to perform the necessary cache flushing, so any
 remaining aliasing issue would be considered a bug.  But the code is
 performance sensitive, some of the problem cases are twisted and complex
 so bugs and unsolved corner cases show up every now and then.

Ok. This means I should also investigate the V4L2 and the hw driver
code, because the cache aliasing shouldn't be there in the first place.

 Does it work for you, even solve your problem?

Sure, with 16 KB page size everything works fine.

-- 
Krzysztof Halasa

Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland
--
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: Suspected cache coherency problem on V4L2 and AR7100 CPU

[Krzysztof Hałasa]

Ralf Baechle r...@linux-mips.org writes:

 That's fine.  You just need to ensure that there are no virtual
 aliases.

Does this include virtual aliasing between a 4 KB TLB-mapped page and
a kseg0 address? I don't really have two TLBs pointing to the same page.

 One way to do so is to increase the page size to 16kB.

Right, this way we will have a unique mapping from the virtual address
to the data cache, as the cache size (per way) is 8 KB here. Is it the
correct fix in this situation?

 Note that there is a variant of the 24K which has a VIPT cache but uses
 hardware to resolve cache aliases.  That is, from a kernel cache management
 perspective it behaves like a PIPT cache.

It seems it's not the case here. What I have here is:
CPU revision is: 00019374 (MIPS 24Kc)
SoC: Atheros AR7161 rev 2
Primary instruction cache 64kB, VIPT, 4-way, linesize 32 bytes.
Primary data cache 32kB, 4-way, VIPT, cache aliases, linesize 32 bytes

 However as I understand what you're mapping to userspace is actually
 device memory, right?

Not exactly - I'm using PCI DMA to userspace SG buffers in RAM.

The userspace first allocates the buffers in normal RAM (with vmalloc()
or something, there is an mmap ioctl() for this), the address returned
is 0x7xxx. Then the buffers (which consist of several pages each)
are presented to the hw driver which obtains separate (kernel) mapping
for each page (kseg0) and does dma_map_sg() and so on. The driver also
simply writes to the buffers. This isn't a problem, though - only the
incoherence between TLB and kseg0 is.

The problem is the userspace doesn't see the kernel writes - The
0x7xxx TLB-mapped pages are read-cached and not invalidated while
the kernel writes to the same pages using kseg0 addresses.

Thanks for looking at this.
-- 
Krzysztof Halasa

Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland
--
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: Suspected cache coherency problem on V4L2 and AR7100 CPU

[Krzysztof Hałasa]

Ralf Baechle r...@linux-mips.org writes:

 That's fine.  You just need to ensure that there are no virtual aliases.
 One way to do so is to increase the page size to 16kB.

Checked, this thing works fine with 16 KB pages.
-- 
Krzysztof Halasa

Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland
--
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: Suspected cache coherency problem on V4L2 and AR7100 CPU

[Krzysztof Hałasa]

Please forgive me my MIPS TLB ignorance.

It seems there is a TLB entry pointing to the userspace buffer at the
time the kernel pointer (kseg0) is used. Is is an allowed situation on
MIPS 24K?

buffer: len 0x1000 (first page),
userspace pointer 0x77327000,
kernel pointer 0x867ac000 (physical address = 0x067ac000)

TLB Index: 15 pgmask=4kb va=77326000 asid=be
   [pa=01149000 c=3 d=1 v=1 g=0] [pa=067ac000 c=3 d=1 v=1 g=0]

Should the TLB entry be deleted before using the kernel pointer (which
points at the same page)?
-- 
Krzysztof Halasa

Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland
--
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

V4L2_BUF_FLAG_NO_CACHE_*

[Krzysztof Hałasa]

Hi,

found them looking for my V4L2 + mmap + MIPS solution... Is the
following intentional? Some out-of-tree code maybe?

$ grep V4L2_BUF_FLAG_NO_CACHE_ * -r

Documentation/DocBook/media/v4l/io.xml: 
entryconstantV4L2_BUF_FLAG_NO_CACHE_INVALIDATE/constant/entry
Documentation/DocBook/media/v4l/io.xml: 
entryconstantV4L2_BUF_FLAG_NO_CACHE_CLEAN/constant/entry
Documentation/DocBook/media/v4l/vidioc-prepare-buf.xml:constantV4L2_BUF_FLAG_NO_CACHE_INVALIDATE/constant
 and
Documentation/DocBook/media/v4l/vidioc-prepare-buf.xml:constantV4L2_BUF_FLAG_NO_CACHE_CLEAN/constant
 flags to skip the respective
include/uapi/linux/videodev2.h:#define V4L2_BUF_FLAG_NO_CACHE_INVALIDATE
0x0800
include/uapi/linux/videodev2.h:#define V4L2_BUF_FLAG_NO_CACHE_CLEAN 
0x1000
-- 
Krzysztof Halasa

Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland
--
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] SOLO6x10: Fix video frame type (I/P/B).

[Krzysztof Hałasa]

Signed-off-by: Krzysztof Hałasa khal...@piap.pl

diff --git a/drivers/staging/media/solo6x10/solo6x10-v4l2-enc.c 
b/drivers/staging/media/solo6x10/solo6x10-v4l2-enc.c
index 7a2fd98..27e9a0a 100644
--- a/drivers/staging/media/solo6x10/solo6x10-v4l2-enc.c
+++ b/drivers/staging/media/solo6x10/solo6x10-v4l2-enc.c
@@ -506,6 +506,7 @@ static int solo_fill_mpeg(struct solo_enc_dev *solo_enc,
return -EIO;
 
/* If this is a key frame, add extra header */
+   vb-v4l2_buf.flags = ~(V4L2_BUF_FLAG_KEYFRAME | V4L2_BUF_FLAG_PFRAME | 
V4L2_BUF_FLAG_BFRAME);
if (!vop_type(vh)) {
skip = solo_enc-vop_len;
vb-v4l2_buf.flags |= V4L2_BUF_FLAG_KEYFRAME;
--
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: Suspected cache coherency problem on V4L2 and AR7100 CPU

[Krzysztof Hałasa]

 I'm debugging a problem with a SOLO6110-based H.264 PCI video encoder on
 Atheros AR7100-based (MIPS, big-endian) platform.

BTW this CPU obviously has VIPT data cache, this means a physical page
with multiple virtual addresses (e.g. mapped multiple times) may and
will be cached multiple times.

AR7100 = arch/mips/ath79.
-- 
Krzysztof Halasa

Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland
--
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

Suspected cache coherency problem on V4L2 and AR7100 CPU

[Krzysztof Hałasa]

Hi,

I'm debugging a problem with a SOLO6110-based H.264 PCI video encoder on
Atheros AR7100-based (MIPS, big-endian) platform.

The problem manifests itself with stale data being returned by the
driver (using ioctl VIDIOC_DQBUF). The stale date always starts and ends
on 32-byte cache line boundary.

The driver is drivers/staging/media/solo6x10.

Initially I thought the encoder hardware is at fault (though it works on
i686 and on (both endians) ARM). But I've eliminated actual DMA accesses
from the driver and the problems still persists.

The control flow is now basically the following:
- userspace program initializes the adapter and allocates 192 KB long
  buffers (at least 2 of them):
open(/dev/video1);

various ioctl() calls

for (cnt = 0; cnt  buffer_count; cnt++) {
struct v4l2_buffer buf = {
.type = V4L2_BUF_TYPE_VIDEO_CAPTURE,
.memory = V4L2_MEMORY_MMAP,
.index = cnt,
};
ioctl(stream-fd, VIDIOC_QUERYBUF, buf);
mmap(NULL, buf.length, PROT_READ | PROT_WRITE, MAP_SHARED, 
stream-fd, buf.m.offset);
}

and then:

for (cnt = 0; cnt  buffer_count; cnt++) {
struct v4l2_buffer buf = {
.type = V4L2_BUF_TYPE_VIDEO_CAPTURE,
.memory = V4L2_MEMORY_MMAP,
.index = cnt,
ioctl(stream-fd, VIDIOC_QBUF, buf);
}

The buffers are now queued. The driver (upon receiving an encoded frame)
now mostly does:

various spin_lock() etc.
vb = list_first_entry(solo_enc-vidq_active, struct solo_vb2_buf, 
list);
list_del(vb-list);

struct vb2_dma_sg_desc *vbuf = vb2_dma_sg_plane_desc(vb, 0);

/* now we have vbuf-sglist which corresponds to a single
userspace 192-KB buffer */

vb2_set_plane_payload(vb, 0, 1024 /* bytes */);

static u32 n = 0; /* a counter to mark each buffer */

/* the following is normally done using dma_map_sg() and DMA,
and also with sg_copy_from_buffer() - eliminated for now */

/* I do the following instead */
struct page *page = sg_page(vbuf-sglist);
u32 *addr = kmap_atomic(page);

/* 4 times as large, I know, the buffer is much longer though */
for (i = 0; i  1024; i++)
addr[i] = n;

flush_dcache_page(page); /* and/or */
flush_kernel_dcache_page(page);

kunmap_atomic(addr);

vb-v4l2_buf.sequence = solo_enc-sequence++;
vb-v4l2_buf.timestamp.tv_sec = vop_sec(vh);
vb-v4l2_buf.timestamp.tv_usec = vop_usec(vh);

vb2_buffer_done(vb, VB2_BUF_STATE_DONE);

The userspace server now does ioctl(VIDIOC_DQBUF), sends it using UDP,
and populates buffer pool again with ioctl(VIDIOC_QBUF).

The driver uses directly-mapped cached (kernel) pointers to access the
buffers (0x8000-0x9FFF kseg0 region) while (obviously)
userspace uses TLB-mapped pointers.

I have verified with a JTAG-based debugger (OpenOCD) that the buffers
are flushed to DRAM (0xAxxx uncached directly-mapped region has
valid data), however the userspace TLB-mapped buffers (which correspond
to the same physical DRAM addresses) partially contain old cached data
(from previous iterations).

The question is which part of the code is at fault, and how do I fix it.
I understand invalidating (and perhaps first flushing) userspace buffers
(cache) should generally fix the problem.

This could also be a simple bug rather than API/platform incompatibility
because usually (though not always) only 1 of the buffers gets corrupted
(the second one of two).

It looks like this - valid buffer, counter n = 0x499, a fragment
of actual UDP packet:
0x0030:   0499  0499  0499  0499  
0x0040:   0499  0499  0499  0499  
0x0050:   0499  0499  0499  0499  
0x0060:   0499  0499  0499  0499  
0x0070:   0499  0499  0499  0499  
0x0080:   0499  0499  0499  0499  
0x0090:   0499  0499  0499  0499  
0x00a0:   0499  0499  0499  0499  
0x00b0:   0499  0499  0499  0499  

next buffer is corrupted, n = 0x49A:
0x0030:   049a  049a  049a  049a  
0x0040:   049a  0468  0468  0468  ...h...h...h
0x0050:   0468  0468  0468  0468  ...h...h...h...h
0x0060:   0468  049a  049a  049a  ...h
0x0070:   049a  049a  049a  049a  
0x0080:   049a  049a  049a  049a  
0x0090: 

[PATCH] SOLO6x10: Remove unused #define SOLO_DEFAULT_GOP

[Krzysztof Hałasa]

Signed-off-by: Krzysztof Hałasa khal...@piap.pl

diff --git a/drivers/staging/media/solo6x10/solo6x10.h 
b/drivers/staging/media/solo6x10/solo6x10.h
index 6f91d2e..f1bbb8c 100644
--- a/drivers/staging/media/solo6x10/solo6x10.h
+++ b/drivers/staging/media/solo6x10/solo6x10.h
@@ -94,7 +94,6 @@
 #define SOLO_ENC_MODE_HD1  1
 #define SOLO_ENC_MODE_D1   9
 
-#define SOLO_DEFAULT_GOP   30
 #define SOLO_DEFAULT_QP3
 
 #ifndef V4L2_BUF_FLAG_MOTION_ON
--
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] SOLO6x10: don't do DMA from stack in solo_dma_vin_region().

[Krzysztof Hałasa]

Signed-off-by: Krzysztof Hałasa khal...@piap.pl

diff --git a/drivers/staging/media/solo6x10/solo6x10-disp.c 
b/drivers/staging/media/solo6x10/solo6x10-disp.c
index 32d9953..884512e 100644
--- a/drivers/staging/media/solo6x10/solo6x10-disp.c
+++ b/drivers/staging/media/solo6x10/solo6x10-disp.c
@@ -176,18 +176,26 @@ static void solo_vout_config(struct solo_dev *solo_dev)
 static int solo_dma_vin_region(struct solo_dev *solo_dev, u32 off,
   u16 val, int reg_size)
 {
-   u16 buf[64];
-   int i;
-   int ret = 0;
+   u16 *buf;
+   const int n = 64, size = n * sizeof(*buf);
+   int i, ret = 0;
+
+   if (!(buf = kmalloc(size, GFP_KERNEL)))
+   return -ENOMEM;
 
-   for (i = 0; i  sizeof(buf)  1; i++)
+   for (i = 0; i  n; i++)
buf[i] = cpu_to_le16(val);
 
-   for (i = 0; i  reg_size; i += sizeof(buf))
-   ret |= solo_p2m_dma(solo_dev, 1, buf,
-   SOLO_MOTION_EXT_ADDR(solo_dev) + off + i,
-   sizeof(buf), 0, 0);
+   for (i = 0; i  reg_size; i += size) {
+   ret = solo_p2m_dma(solo_dev, 1, buf,
+  SOLO_MOTION_EXT_ADDR(solo_dev) + off + i,
+  size, 0, 0);
+
+   if (ret)
+   break;
+   }
 
+   kfree(buf);
return ret;
 }
 
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] SOLO6x10: Fix video encoding on big-endian systems.

[Krzysztof Hałasa]

Signed-off-by: Krzysztof Hałasa khal...@piap.pl

diff --git a/drivers/staging/media/solo6x10/solo6x10-v4l2-enc.c 
b/drivers/staging/media/solo6x10/solo6x10-v4l2-enc.c
index a4c5896..e501287 100644
--- a/drivers/staging/media/solo6x10/solo6x10-v4l2-enc.c
+++ b/drivers/staging/media/solo6x10/solo6x10-v4l2-enc.c
@@ -95,38 +95,11 @@ static unsigned char vop_6110_pal_cif[] = {
0x01, 0x68, 0xce, 0x32, 0x28, 0x00, 0x00, 0x00,
 };
 
-struct vop_header {
-   /* VE_STATUS0 */
-   u32 mpeg_size:20, sad_motion_flag:1, video_motion_flag:1, vop_type:2,
-   channel:5, source_fl:1, interlace:1, progressive:1;
-
-   /* VE_STATUS1 */
-   u32 vsize:8, hsize:8, last_queue:4, nop0:8, scale:4;
-
-   /* VE_STATUS2 */
-   u32 mpeg_off;
-
-   /* VE_STATUS3 */
-   u32 jpeg_off;
-
-   /* VE_STATUS4 */
-   u32 jpeg_size:20, interval:10, nop1:2;
-
-   /* VE_STATUS5/6 */
-   u32 sec, usec;
-
-   /* VE_STATUS7/8/9 */
-   u32 nop2[3];
-
-   /* VE_STATUS10 */
-   u32 mpeg_size_alt:20, nop3:12;
-
-   u32 end_nops[5];
-} __packed;
+typedef __le32 vop_header[16];
 
 struct solo_enc_buf {
enum solo_enc_types type;
-   struct vop_header   *vh;
+   const vop_header*vh;
int motion;
 };
 
@@ -430,8 +403,64 @@ static int solo_send_desc(struct solo_enc_dev *solo_enc, 
int skip,
 solo_enc-desc_count - 1);
 }
 
+/* Extract values from VOP header - VE_STATUSxx */
+static inline int vop_interlaced(const vop_header *vh)
+{
+   return (__le32_to_cpu((*vh)[0])  30)  1;
+}
+
+static inline u8 vop_channel(const vop_header *vh)
+{
+   return (__le32_to_cpu((*vh)[0])  24)  0x1F;
+}
+
+static inline u8 vop_type(const vop_header *vh)
+{
+   return (__le32_to_cpu((*vh)[0])  22)  3;
+}
+
+static inline u32 vop_mpeg_size(const vop_header *vh)
+{
+   return __le32_to_cpu((*vh)[0])  0xF;
+}
+
+static inline u8 vop_hsize(const vop_header *vh)
+{
+   return (__le32_to_cpu((*vh)[1])  8)  0xFF;
+}
+
+static inline u8 vop_vsize(const vop_header *vh)
+{
+   return __le32_to_cpu((*vh)[1])  0xFF;
+}
+
+static inline u32 vop_mpeg_offset(const vop_header *vh)
+{
+   return __le32_to_cpu((*vh)[2]);
+}
+
+static inline u32 vop_jpeg_offset(const vop_header *vh)
+{
+   return __le32_to_cpu((*vh)[3]);
+}
+
+static inline u32 vop_jpeg_size(const vop_header *vh)
+{
+   return __le32_to_cpu((*vh)[4])  0xF;
+}
+
+static inline u32 vop_sec(const vop_header *vh)
+{
+   return __le32_to_cpu((*vh)[5]);
+}
+
+static inline u32 vop_usec(const vop_header *vh)
+{
+   return __le32_to_cpu((*vh)[6]);
+}
+
 static int solo_fill_jpeg(struct solo_enc_dev *solo_enc,
-   struct vb2_buffer *vb, struct vop_header *vh)
+ struct vb2_buffer *vb, const vop_header *vh)
 {
struct solo_dev *solo_dev = solo_enc-solo_dev;
struct vb2_dma_sg_desc *vbuf = vb2_dma_sg_plane_desc(vb, 0);
@@ -440,29 +469,30 @@ static int solo_fill_jpeg(struct solo_enc_dev *solo_enc,
 
vb-v4l2_buf.flags |= V4L2_BUF_FLAG_KEYFRAME;
 
-   if (vb2_plane_size(vb, 0)  vh-jpeg_size + solo_enc-jpeg_len)
+   if (vb2_plane_size(vb, 0)  vop_jpeg_size(vh) + solo_enc-jpeg_len)
return -EIO;
 
sg_copy_from_buffer(vbuf-sglist, vbuf-num_pages,
solo_enc-jpeg_header,
solo_enc-jpeg_len);
 
-   frame_size = (vh-jpeg_size + solo_enc-jpeg_len + (DMA_ALIGN - 1))
+   frame_size = (vop_jpeg_size(vh) + solo_enc-jpeg_len + (DMA_ALIGN - 1))
 ~(DMA_ALIGN - 1);
-   vb2_set_plane_payload(vb, 0, vh-jpeg_size + solo_enc-jpeg_len);
+   vb2_set_plane_payload(vb, 0, vop_jpeg_size(vh) + solo_enc-jpeg_len);
 
dma_map_sg(solo_dev-pdev-dev, vbuf-sglist, vbuf-num_pages,
DMA_FROM_DEVICE);
-   ret = solo_send_desc(solo_enc, solo_enc-jpeg_len, vbuf, vh-jpeg_off,
-   frame_size, SOLO_JPEG_EXT_ADDR(solo_dev),
-   SOLO_JPEG_EXT_SIZE(solo_dev));
+   ret = solo_send_desc(solo_enc, solo_enc-jpeg_len, vbuf,
+vop_jpeg_offset(vh) - SOLO_JPEG_EXT_ADDR(solo_dev),
+frame_size, SOLO_JPEG_EXT_ADDR(solo_dev),
+SOLO_JPEG_EXT_SIZE(solo_dev));
dma_unmap_sg(solo_dev-pdev-dev, vbuf-sglist, vbuf-num_pages,
DMA_FROM_DEVICE);
return ret;
 }
 
 static int solo_fill_mpeg(struct solo_enc_dev *solo_enc,
-   struct vb2_buffer *vb, struct vop_header *vh)
+   struct vb2_buffer *vb, const vop_header *vh)
 {
struct solo_dev *solo_dev = solo_enc-solo_dev;
struct vb2_dma_sg_desc *vbuf = vb2_dma_sg_plane_desc(vb, 0);
@@ -470,11 +500,11 @@ static int solo_fill_mpeg(struct solo_enc_dev *solo_enc,
int skip = 0;
int ret;
 
-   if (vb2_plane_size(vb, 0)  vh

[PATCH] SOLO6x10: Fix video headers on certain hardware.

[Krzysztof Hałasa]

On certain platforms a sequence of dma_map_sg() and dma_unmap_sg()
discards data previously stored in the buffers. Build video headers
only after the DMA is completed.

Signed-off-by: Krzysztof Hałasa khal...@piap.pl

diff --git a/drivers/staging/media/solo6x10/solo6x10-v4l2-enc.c 
b/drivers/staging/media/solo6x10/solo6x10-v4l2-enc.c
index e501287..7a2fd98 100644
--- a/drivers/staging/media/solo6x10/solo6x10-v4l2-enc.c
+++ b/drivers/staging/media/solo6x10/solo6x10-v4l2-enc.c
@@ -472,14 +472,11 @@ static int solo_fill_jpeg(struct solo_enc_dev *solo_enc,
if (vb2_plane_size(vb, 0)  vop_jpeg_size(vh) + solo_enc-jpeg_len)
return -EIO;
 
-   sg_copy_from_buffer(vbuf-sglist, vbuf-num_pages,
-   solo_enc-jpeg_header,
-   solo_enc-jpeg_len);
-
frame_size = (vop_jpeg_size(vh) + solo_enc-jpeg_len + (DMA_ALIGN - 1))
 ~(DMA_ALIGN - 1);
vb2_set_plane_payload(vb, 0, vop_jpeg_size(vh) + solo_enc-jpeg_len);
 
+   /* may discard all previous data in vbuf-sglist */
dma_map_sg(solo_dev-pdev-dev, vbuf-sglist, vbuf-num_pages,
DMA_FROM_DEVICE);
ret = solo_send_desc(solo_enc, solo_enc-jpeg_len, vbuf,
@@ -488,6 +485,11 @@ static int solo_fill_jpeg(struct solo_enc_dev *solo_enc,
 SOLO_JPEG_EXT_SIZE(solo_dev));
dma_unmap_sg(solo_dev-pdev-dev, vbuf-sglist, vbuf-num_pages,
DMA_FROM_DEVICE);
+
+   /* add the header only after dma_unmap_sg() */
+   sg_copy_from_buffer(vbuf-sglist, vbuf-num_pages,
+   solo_enc-jpeg_header, solo_enc-jpeg_len);
+
return ret;
 }
 
@@ -505,12 +507,7 @@ static int solo_fill_mpeg(struct solo_enc_dev *solo_enc,
 
/* If this is a key frame, add extra header */
if (!vop_type(vh)) {
-   sg_copy_from_buffer(vbuf-sglist, vbuf-num_pages,
-   solo_enc-vop,
-   solo_enc-vop_len);
-
skip = solo_enc-vop_len;
-
vb-v4l2_buf.flags |= V4L2_BUF_FLAG_KEYFRAME;
vb2_set_plane_payload(vb, 0, vop_mpeg_size(vh) + 
solo_enc-vop_len);
} else {
@@ -524,6 +521,7 @@ static int solo_fill_mpeg(struct solo_enc_dev *solo_enc,
frame_size = (vop_mpeg_size(vh) + skip + (DMA_ALIGN - 1))
 ~(DMA_ALIGN - 1);
 
+   /* may discard all previous data in vbuf-sglist */
dma_map_sg(solo_dev-pdev-dev, vbuf-sglist, vbuf-num_pages,
DMA_FROM_DEVICE);
ret = solo_send_desc(solo_enc, skip, vbuf, frame_off, frame_size,
@@ -531,6 +529,11 @@ static int solo_fill_mpeg(struct solo_enc_dev *solo_enc,
SOLO_MP4E_EXT_SIZE(solo_dev));
dma_unmap_sg(solo_dev-pdev-dev, vbuf-sglist, vbuf-num_pages,
DMA_FROM_DEVICE);
+
+   /* add the header only after dma_unmap_sg() */
+   if (!vop_type(vh))
+   sg_copy_from_buffer(vbuf-sglist, vbuf-num_pages,
+   solo_enc-vop, solo_enc-vop_len);
return ret;
 }
 
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

SOLO6x10 MPEG4/H.264 encoder driver

[Krzysztof Hałasa]

Hi Hans,

I'm trying to move to Linux 3.11 and I noticed you've made some
significant changes to the SOLO6x10 driver. While I don't yet have
the big picture, I can see some regressions here:

- the driver doesn't even try to work on big endian systems (I'm using
  IXP4xx-based system which is BE). For instance, you're now using
  bitfields for frame headers (struct vop_header) and this is well known
  to fail (unless you have a different struct for each endianness).

  This is actually what I have fixed with commit
  c55564fdf793797dd2740a67c35a2cedc133c9ff in 2011, and you brought the
  old buggy version back with dcae5dacbce518513abf7776cb450b7bd95d722b.

- you removed my dynamic building of MPEG4/H.264 VOP headers (the same
  commit c55564fdf793797dd2740a67c35a2cedc133c9ff) and replaced it with
  precomputed static binary headers, one for each PAL/NTSC/D1/CIF
  combination. While I don't strictly object the precomputed data,
  perhaps you could consider adding some tool to optionally calculate
  them, as required by the license. For now, It seems it's practically
  impossible to make modifications to the header data, without, for
  example, extracting the code from older driver version.

- what was the motivation behind renaming all (C language) files in
  drivers/staging/media/solo6x10 to solo6x10-* (commits
  dad7fab933622ee67774a9219d5c18040d97a5e5 and
  7bce33daeaca26a3ea3f6099fdfe4e11ea46cac6, essentially a reversion of
  my commit ae69b22c6ca7d1d7fdccf0b664dafbc777099abe)? I'm under
  impression that a driver file names don't need (and shouldn't) contain
  the driver name if the directory is already named after the driver.

  This is also the case with b3c7d453a00b7dadc2a7435f68b012371ccc3a3e:

   [media] solo6x10: rename jpeg.h to solo6x10-jpeg.h
  
This header clashes with the jpeg.h in gspca when doing a compatibility
build using the media_build system.

  What is this media_build system and why is it forcing code in
  different directories to have unique file names?


I appreciate the switch to VB2 and other improvements (though I can't
test them yet), but perhaps it could be done without causing major
breakage?

I'm thinking about a correct course of action now. I need the driver
functional so I'll revert the struct vop_header thing again, any
thoughts?
-- 
Krzysztof Halasa

Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland
--
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: SOLO6x10 MPEG4/H.264 encoder driver

[Krzysztof Hałasa]

Hans Verkuil hverk...@xs4all.nl writes:

 I took the latest bluecherry code as the basis for the changes, merging what
 I could from the kernel code. Unfortunately this was very hard to do backport,
 so I decided to take bluecherry's code.

I see, thanks for speedy explanation.

If I may suggest something (especially to Ismael), perhaps we do the
further development here, I mean based on git.kernel.org sources, and
not on (unsynced) bluecherry's.

I will fix this stuff again.
-- 
Krzysztof Halasa

Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland
--
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