[GIT,PULL] ViewCast O820E capture support added

  Hi Mauro,

A new PCIe bridge driver below. It was released a couple of months ago
to the public, probably about time
we got this into the request queue. I'll review the linux-firmware
additions shortly, I have a firmware blob
and flexible license for the distros.

The following changes since commit da2cd767f537082be0a02d83f87e0da4270e25b2:

  [media] ttpci: add support for Omicom S2 PCI (2012-08-12 14:41:26 -0300)

are available in the git repository at:
  git://git.kernellabs.com/stoth/media_tree.git o820e

Steven Toth (1):
      [media] vc8x0: Add support for the ViewCast O820E card.

 drivers/media/video/Kconfig                 |    2 +
 drivers/media/video/Makefile                |    1 +
 drivers/media/video/vc8x0/Kconfig           |   14 +
 drivers/media/video/vc8x0/Makefile          |   10 +
 drivers/media/video/vc8x0/vc8x0-ad7441.c    | 3057 +++++++++++++++++++++++++++
 drivers/media/video/vc8x0/vc8x0-audio.c     |  736 +++++++
 drivers/media/video/vc8x0/vc8x0-buffer.c    |  338 +++
 drivers/media/video/vc8x0/vc8x0-cards.c     |  138 ++
 drivers/media/video/vc8x0/vc8x0-channel.c   |  934 ++++++++
 drivers/media/video/vc8x0/vc8x0-core.c      |  887 ++++++++
 drivers/media/video/vc8x0/vc8x0-display.c   | 1359 ++++++++++++
 drivers/media/video/vc8x0/vc8x0-dma.c       | 2677 +++++++++++++++++++++++
 drivers/media/video/vc8x0/vc8x0-eeprom.c    |   71 +
 drivers/media/video/vc8x0/vc8x0-fw.c        |  429 ++++
 drivers/media/video/vc8x0/vc8x0-i2c.c       |  290 +++
 drivers/media/video/vc8x0/vc8x0-pcm3052.c   |  192 ++
 drivers/media/video/vc8x0/vc8x0-reg.h       |  214 ++
 drivers/media/video/vc8x0/vc8x0-timestamp.c |  156 ++
 drivers/media/video/vc8x0/vc8x0-vga.c       |  430 ++++
 drivers/media/video/vc8x0/vc8x0-video.c     | 2650 +++++++++++++++++++++++
 drivers/media/video/vc8x0/vc8x0.h           |  995 +++++++++
 21 files changed, 15580 insertions(+), 0 deletions(-)
 create mode 100644 drivers/media/video/vc8x0/Kconfig
 create mode 100644 drivers/media/video/vc8x0/Makefile
 create mode 100644 drivers/media/video/vc8x0/vc8x0-ad7441.c
 create mode 100644 drivers/media/video/vc8x0/vc8x0-audio.c
 create mode 100644 drivers/media/video/vc8x0/vc8x0-buffer.c
 create mode 100644 drivers/media/video/vc8x0/vc8x0-cards.c
 create mode 100644 drivers/media/video/vc8x0/vc8x0-channel.c
 create mode 100644 drivers/media/video/vc8x0/vc8x0-core.c
 create mode 100644 drivers/media/video/vc8x0/vc8x0-display.c
 create mode 100644 drivers/media/video/vc8x0/vc8x0-dma.c
 create mode 100644 drivers/media/video/vc8x0/vc8x0-eeprom.c
 create mode 100644 drivers/media/video/vc8x0/vc8x0-fw.c
 create mode 100644 drivers/media/video/vc8x0/vc8x0-i2c.c
 create mode 100644 drivers/media/video/vc8x0/vc8x0-pcm3052.c
 create mode 100644 drivers/media/video/vc8x0/vc8x0-reg.h
 create mode 100644 drivers/media/video/vc8x0/vc8x0-timestamp.c
 create mode 100644 drivers/media/video/vc8x0/vc8x0-vga.c
 create mode 100644 drivers/media/video/vc8x0/vc8x0-video.c
 create mode 100644 drivers/media/video/vc8x0/vc8x0.h

Regards,

- Steve
  

Hi Steve!

On Mon August 13 2012 01:16:30 Steven Toth wrote:
> Hi Mauro,
> 
> A new PCIe bridge driver below. It was released a couple of months ago
> to the public, probably about time
> we got this into the request queue. I'll review the linux-firmware
> additions shortly, I have a firmware blob
> and flexible license for the distros.

I went through this driver from a high-level point of view, and I'm afraid I
have quite a number of issues with this driver.

One of the bigger ones is that vc8x0-ad7441.c should be implemented as
a subdevice. I have two other AD drivers in my queue (adv7604 and ad9389b),
so you can look at those for comparison.

See: http://www.spinics.net/lists/linux-media/msg51501.html

These Analog Devices chips are quite complex, and you really want to be able
to reuse drivers.

Some of the other issues are:

- Please use the control framework. All new drivers must use it, unless there
  are very, very good reasons not to. I'm gradually converting all drivers to
  the control framework, so I really don't want to introduce new drivers to
  that list.

- TRY_FMT can actually set the format, something that should never happen.

- Use the new DV_TIMINGS ioctls for the HDTV formats. S_FMT should not be used
  to select the HDTV format!

- The procfs additions seem unnecessary to me. VIDIOC_LOG_STATUS or perhaps
  debugfs are probably much more suitable.

- Using videobuf2 is very much recommended.

- Please run v4l2-compliance and fix any reported issues!

It's a pretty big driver, so I only looked skimmed the patch, but these are
IMHO fairly major issues. As it stands it is only suitable to be merged in
drivers/staging/media.

Regards,

	Hans

> 
> The following changes since commit da2cd767f537082be0a02d83f87e0da4270e25b2:
> 
>   [media] ttpci: add support for Omicom S2 PCI (2012-08-12 14:41:26 -0300)
> 
> are available in the git repository at:
>   git://git.kernellabs.com/stoth/media_tree.git o820e
> 
> Steven Toth (1):
>       [media] vc8x0: Add support for the ViewCast O820E card.
> 
>  drivers/media/video/Kconfig                 |    2 +
>  drivers/media/video/Makefile                |    1 +
>  drivers/media/video/vc8x0/Kconfig           |   14 +
>  drivers/media/video/vc8x0/Makefile          |   10 +
>  drivers/media/video/vc8x0/vc8x0-ad7441.c    | 3057 +++++++++++++++++++++++++++
>  drivers/media/video/vc8x0/vc8x0-audio.c     |  736 +++++++
>  drivers/media/video/vc8x0/vc8x0-buffer.c    |  338 +++
>  drivers/media/video/vc8x0/vc8x0-cards.c     |  138 ++
>  drivers/media/video/vc8x0/vc8x0-channel.c   |  934 ++++++++
>  drivers/media/video/vc8x0/vc8x0-core.c      |  887 ++++++++
>  drivers/media/video/vc8x0/vc8x0-display.c   | 1359 ++++++++++++
>  drivers/media/video/vc8x0/vc8x0-dma.c       | 2677 +++++++++++++++++++++++
>  drivers/media/video/vc8x0/vc8x0-eeprom.c    |   71 +
>  drivers/media/video/vc8x0/vc8x0-fw.c        |  429 ++++
>  drivers/media/video/vc8x0/vc8x0-i2c.c       |  290 +++
>  drivers/media/video/vc8x0/vc8x0-pcm3052.c   |  192 ++
>  drivers/media/video/vc8x0/vc8x0-reg.h       |  214 ++
>  drivers/media/video/vc8x0/vc8x0-timestamp.c |  156 ++
>  drivers/media/video/vc8x0/vc8x0-vga.c       |  430 ++++
>  drivers/media/video/vc8x0/vc8x0-video.c     | 2650 +++++++++++++++++++++++
>  drivers/media/video/vc8x0/vc8x0.h           |  995 +++++++++
>  21 files changed, 15580 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/media/video/vc8x0/Kconfig
>  create mode 100644 drivers/media/video/vc8x0/Makefile
>  create mode 100644 drivers/media/video/vc8x0/vc8x0-ad7441.c
>  create mode 100644 drivers/media/video/vc8x0/vc8x0-audio.c
>  create mode 100644 drivers/media/video/vc8x0/vc8x0-buffer.c
>  create mode 100644 drivers/media/video/vc8x0/vc8x0-cards.c
>  create mode 100644 drivers/media/video/vc8x0/vc8x0-channel.c
>  create mode 100644 drivers/media/video/vc8x0/vc8x0-core.c
>  create mode 100644 drivers/media/video/vc8x0/vc8x0-display.c
>  create mode 100644 drivers/media/video/vc8x0/vc8x0-dma.c
>  create mode 100644 drivers/media/video/vc8x0/vc8x0-eeprom.c
>  create mode 100644 drivers/media/video/vc8x0/vc8x0-fw.c
>  create mode 100644 drivers/media/video/vc8x0/vc8x0-i2c.c
>  create mode 100644 drivers/media/video/vc8x0/vc8x0-pcm3052.c
>  create mode 100644 drivers/media/video/vc8x0/vc8x0-reg.h
>  create mode 100644 drivers/media/video/vc8x0/vc8x0-timestamp.c
>  create mode 100644 drivers/media/video/vc8x0/vc8x0-vga.c
>  create mode 100644 drivers/media/video/vc8x0/vc8x0-video.c
>  create mode 100644 drivers/media/video/vc8x0/vc8x0.h
> 
> Regards,
> 
> - Steve
> 
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  

Hans,

Thanks for your feedback.

Oh dear. I don't think you're going to like my response, but I think
we know each other well enough to realize that neither of us are
trying to antagonize or upset either other. We're simply stating our
positions. Please read on.

> I went through this driver from a high-level point of view, and I'm afraid I
> have quite a number of issues with this driver.
>
> One of the bigger ones is that vc8x0-ad7441.c should be implemented as
> a subdevice. I have two other AD drivers in my queue (adv7604 and ad9389b),
> so you can look at those for comparison.
>
> See: http://www.spinics.net/lists/linux-media/msg51501.html

Oh, I understand how to write video decoder drivers. I just chose
specifically not to do it. This was intensional on my part. If when
another card comes along with an ad7441 when they are welcome to split
the code and/or create the subdevs. It's extra engineering today that
does't improve the support for the 820 card and doesn't benefit any
other known product. It's a feature that's exclusive to the 820.
Future developers are welcome to fork, slice and/or copy the code.

Today, the driver is tuned exactly for the card, it works very nicely
for end users and the code is easy to read, simple to debug and relies
on only a handful of v4l2 framework apis.

If anyone knew how much time and suffering I've had with the cx25840
over the years, why I think has gotten worse with the subdev controls,
you'd be shocked. The subdev framework (in my opinion) has become
unwieldily and difficult to debug, based on my recent experience
dealing with some cx23885 issues. I'm intensionally trying not to use
it.

I should be clear, my comments are not mean to antagonize or inflame,
I'm simply pointing out that when at all possible I chose not to use
the subdev framework because if it's delays and difficulties when it
comes to debugging.

When did the subdev framework become a mandatory requirement for any
driver merge?

>
> These Analog Devices chips are quite complex, and you really want to be able
> to reuse drivers.

I am certainly more than willing to discuss re-use, when re-use make
sense. Right now we have no-practical re-use for this part to speak
of. The code is targeted towards the 820, in the use case that end
users need. If someone would like to build a ad7441 subdevice and use
that in their driver then they are welcome to the code. In practise,
sharing complex video decoders across driver designs leads to massive
regressions, as witnessed on the list this year.

I am also aware that the cx25840 driver is complex, and the end result
was that driver maintainers effectively forked the cx25840 and brought
the codebase back into their core drivers (cx18 for example), to avoid
issues where regression testing was troublesome across so many cards.
If I had the time and/or energy, we'd do the same with the cx25840.
Keeping complex code close to the PCI/PCIe bridge bring big dividends
when debugging complex problems and mitigating issues where code is
re-used across multiple products (growing any regression test
requirements).

The entire driver was intensionally written to be self-enclosed,
highly portable between 2.6.3x and v3.x kernels without subdev
breakages and/or api changes. With a small external Makefile it even
builds very nicely outside of the kernel on any kernel you like, that
shipped in the last 2-3 years.

>
> Some of the other issues are:
>
> - Please use the control framework. All new drivers must use it, unless there
>   are very, very good reasons not to. I'm gradually converting all drivers to
>   the control framework, so I really don't want to introduce new drivers to
>   that list.

I think the control framework is a great design, it's just too
difficult to debug and when I go near it - it breaks, or I spend an
hour trying to understand why my subdev call doesn't reach the subdev
device. My comments are not designed to inflame or upset you, I'm
simply pointing out that any work I've done recently (on two new PCIe
bridges - unreleased code) I've decided not to use it.

Again, I specifically chosen to isolate this driver from certain key
areas of the (now enormous) v4l2 infrastructure.

>
> - TRY_FMT can actually set the format, something that should never happen.

I can check, but I think gstreamer or tvtime actually relies on that behavior.

>
> - Use the new DV_TIMINGS ioctls for the HDTV formats. S_FMT should not be used
>   to select the HDTV format!

gstreamer on 2.6.37 and better didn't support DVTIMINGs. I would
certainly like to discuss adding better timing support once
applications are fully aware and can control the hardware using it.
The lack of a good timin API (and adoption by the application
developers) forced my hand to use S_FMT.

Right now the driver works today, on old and new systems, for hardware
that's shipping. It satisfies end user needs.

>
> - The procfs additions seem unnecessary to me. VIDIOC_LOG_STATUS or perhaps
>   debugfs are probably much more suitable.

I agree. It can probably be removed altogether.

>
> - Using videobuf2 is very much recommended.

I went with what I know to be honest. I neither agree nor disagree
with your comments. If videobuf2 is supported on 2.6.3x then this is
good news.

>
> - Please run v4l2-compliance and fix any reported issues!
>
> It's a pretty big driver, so I only looked skimmed the patch, but these are
> IMHO fairly major issues. As it stands it is only suitable to be merged in
> drivers/staging/media.

I'm not sure I agree. I think I don't agree in general that subdev and
the control framework is mandatory for any driver. I think they are
accelerator frameworks designed to help. I my case I don't think they
do. So I avoided using them.

I guess Mauro has the final decision.
  

On Mon August 13 2012 16:46:45 Steven Toth wrote:
> Hans,
> 
> Thanks for your feedback.
> 
> Oh dear. I don't think you're going to like my response, but I think
> we know each other well enough to realize that neither of us are
> trying to antagonize or upset either other. We're simply stating our
> positions. Please read on.

I didn't think you'd like my response either :-)

> > I went through this driver from a high-level point of view, and I'm afraid I
> > have quite a number of issues with this driver.
> >
> > One of the bigger ones is that vc8x0-ad7441.c should be implemented as
> > a subdevice. I have two other AD drivers in my queue (adv7604 and ad9389b),
> > so you can look at those for comparison.
> >
> > See: http://www.spinics.net/lists/linux-media/msg51501.html
> 
> Oh, I understand how to write video decoder drivers. I just chose
> specifically not to do it. This was intensional on my part. If when
> another card comes along with an ad7441 when they are welcome to split
> the code and/or create the subdevs. It's extra engineering today that
> does't improve the support for the 820 card and doesn't benefit any
> other known product. It's a feature that's exclusive to the 820.
> Future developers are welcome to fork, slice and/or copy the code.
> Today, the driver is tuned exactly for the card, it works very nicely
> for end users and the code is easy to read, simple to debug and relies
> on only a handful of v4l2 framework apis.

The problem is that it is very hard, if not impossible, to split something
up after the fact. There are almost no drivers left that still have an
integral i2c driver as opposed to a separate i2c driver. I only know of
a few old ones and at least one in staging (not counting reverse
engineered drivers such as in gspca where there is no choice).

The main advantage is that it is much easier to extend the functionality
of your driver, particularly for devices with a lot of functionality like
those ADV drivers.

> If anyone knew how much time and suffering I've had with the cx25840
> over the years, why I think has gotten worse with the subdev controls,
> you'd be shocked.

As someone who has been heavily involved with the cx25840 I know how you
feel. However, in my opinion that is mostly caused by the cx25840 hardware
itself which is a piece of sh*t. Most other subdevices see much less change
and are not as sensitive to breakage as that one.

> The subdev framework (in my opinion) has become
> unwieldily and difficult to debug, based on my recent experience
> dealing with some cx23885 issues. I'm intensionally trying not to use
> it.

In what respect does the subdev framework make debugging hard? I'd like to
know about it so we can try and improve it.

The subdev framework was made to make reuse possible. If everyone refuses
to use it, then we're back to the bad old times where you get 20
almost-but-not-quite identical i2c driver implementations.

> I should be clear, my comments are not mean to antagonize or inflame,
> I'm simply pointing out that when at all possible I chose not to use
> the subdev framework because if it's delays and difficulties when it
> comes to debugging.
> 
> When did the subdev framework become a mandatory requirement for any
> driver merge?

It is certainly highly recommended, and in my opinion there should be
very good reasons for not doing it. In the end it is Mauro who decides,
although I personally would be in favor of mandating it. Not to pester
people, but it is so much harder to split it up after the fact. Just like
the fact that documentation is now required if a new API is added, because
nobody ever writes documentation afterwards.

> >
> > These Analog Devices chips are quite complex, and you really want to be able
> > to reuse drivers.
> 
> I am certainly more than willing to discuss re-use, when re-use make
> sense. Right now we have no-practical re-use for this part to speak
> of. The code is targeted towards the 820, in the use case that end
> users need. If someone would like to build a ad7441 subdevice and use
> that in their driver then they are welcome to the code. In practise,
> sharing complex video decoders across driver designs leads to massive
> regressions, as witnessed on the list this year.
> 
> I am also aware that the cx25840 driver is complex, and the end result
> was that driver maintainers effectively forked the cx25840 and brought
> the codebase back into their core drivers (cx18 for example), to avoid
> issues where regression testing was troublesome across so many cards.

I don't think the cx18 ever used the cx25840 codebase: while the cx23418
uses a similar IP as the cx25840 there were too many differences to allow
us (actually, I think it was me who wrote it initially for the cx18) to
reuse the cx25840.

That's a general problem, BTW: reuse works well for the exact same chip.
But when you get variants of the chip (or it gets integrated in another chip
as an IP block), then at some point keeping track of those differences and
preventing regressions becomes harder than it would be if they were done as
separate drivers.

Where that boundary is is not always clear, and for the cx25840 we probably
exceeded it.

But sharing a subdevice driver for a single chip among different boards has
never been much of a problem in my experience. I'm not saying we never got
regressions, but not many and they were generally quickly caught.

> If I had the time and/or energy, we'd do the same with the cx25840.
> Keeping complex code close to the PCI/PCIe bridge bring big dividends
> when debugging complex problems and mitigating issues where code is
> re-used across multiple products (growing any regression test
> requirements).
> 
> The entire driver was intensionally written to be self-enclosed,
> highly portable between 2.6.3x and v3.x kernels without subdev
> breakages and/or api changes. With a small external Makefile it even
> builds very nicely outside of the kernel on any kernel you like, that
> shipped in the last 2-3 years.

??? We have the media_build system for that, so why care about older kernels?

> >
> > Some of the other issues are:
> >
> > - Please use the control framework. All new drivers must use it, unless there
> >   are very, very good reasons not to. I'm gradually converting all drivers to
> >   the control framework, so I really don't want to introduce new drivers to
> >   that list.
> 
> I think the control framework is a great design, it's just too
> difficult to debug and when I go near it - it breaks, or I spend an
> hour trying to understand why my subdev call doesn't reach the subdev
> device. My comments are not designed to inflame or upset you, I'm
> simply pointing out that any work I've done recently (on two new PCIe
> bridges - unreleased code) I've decided not to use it.

Ask me if there are problems with the control framework! I'm happy to help out.

I can't fix it, improve it, etc. if I don't here about it.

The reason why all drivers should use it is that is behaves the same for all
drivers, and apps can start to rely on that behavior. If something doesn't work,
and you can't figure it out, then I am more than happy to help.

But I will NACK this driver unless it is using the control framework. Otherwise
I just have to do that later, and I really don't want to. There aren't many
controls in this driver so it should be pretty quick to do it.

In addition, the control framework will make it easy to implement control events,
another thing that should be rolled out to all drivers.

> Again, I specifically chosen to isolate this driver from certain key
> areas of the (now enormous) v4l2 infrastructure.

There is a reason for that infrastructure, you know. Consistent behavior from
the point of view of applications is the most important one. And it actually
takes a lot of work off your hands. Again, if there are questions, then I'm
happy to help out (and possible improve the code or documentation).

> 
> >
> > - TRY_FMT can actually set the format, something that should never happen.
> 
> I can check, but I think gstreamer or tvtime actually relies on that behavior.

Highly unlikely. TRY_FMT should *never* change the actual format. It's been like
that since the very beginning.

> 
> >
> > - Use the new DV_TIMINGS ioctls for the HDTV formats. S_FMT should not be used
> >   to select the HDTV format!
> 
> gstreamer on 2.6.37 and better didn't support DVTIMINGs. I would
> certainly like to discuss adding better timing support once
> applications are fully aware and can control the hardware using it.
> The lack of a good timin API (and adoption by the application
> developers) forced my hand to use S_FMT.

You said that it was in your queue for some time, so it may be that it wasn't
available when you started out. The problem is, the API is now available, and
if drivers do not implement it, then there is also no reason for applications
to use it, isn't it? Chicken and egg.

I will NACK if the driver doesn't add support for it. Otherwise we end up with
some drivers that implement it correctly, and others that use a different
method. No application writer will ever thank us for that.

Also, I didn't go through all the hard work of designing and adding an API and
then see it being ignored in favor of a non-standard solution.

> Right now the driver works today, on old and new systems, for hardware
> that's shipping. It satisfies end user needs.
> 
> >
> > - The procfs additions seem unnecessary to me. VIDIOC_LOG_STATUS or perhaps
> >   debugfs are probably much more suitable.
> 
> I agree. It can probably be removed altogether.
> 
> >
> > - Using videobuf2 is very much recommended.
> 
> I went with what I know to be honest. I neither agree nor disagree
> with your comments. If videobuf2 is supported on 2.6.3x then this is
> good news.

If you use media_build to compile your driver, then everything including vb2
is supported from 2.6.31 onwards (the oldest kernel supported by media_build).

> 
> >
> > - Please run v4l2-compliance and fix any reported issues!
> >
> > It's a pretty big driver, so I only looked skimmed the patch, but these are
> > IMHO fairly major issues. As it stands it is only suitable to be merged in
> > drivers/staging/media.
> 
> I'm not sure I agree. I think I don't agree in general that subdev and
> the control framework is mandatory for any driver. I think they are
> accelerator frameworks designed to help. I my case I don't think they
> do. So I avoided using them.
>
> I guess Mauro has the final decision.

Of course.

BTW, I saw another thing that must be changed: you use the .ioctl file
operation instead of unlocked_ioctl. This is no longer allowed for new
drivers since the removal of the BKL. The v4l2 core implementation of
.ioctl attempts to simulate the old BKL and is very inefficient, in
particular for drivers like this that do not use struct v4l2_device.
If you have multiple ViewCast cards, then all v4l2 ioctls will go through
a single V4L2 core lock, leading to substantial latencies.

See also the comment in v4l2_ioctl() in v4l2_dev.c.

New drivers must use unlocked_ioctl. I am in the (very slow, but steady)
process of fixing any old drivers that still use .ioctl and once all are
converted .ioctl will be removed altogether.

Regards,

	Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  

Em 13-08-2012 12:49, Hans Verkuil escreveu:
> On Mon August 13 2012 16:46:45 Steven Toth wrote:
>> Hans,
>>
>> Thanks for your feedback.
>>
>> Oh dear. I don't think you're going to like my response, but I think
>> we know each other well enough to realize that neither of us are
>> trying to antagonize or upset either other. We're simply stating our
>> positions. Please read on.
> 
> I didn't think you'd like my response either :-)

You probably won't like my answer too, yet I'm also simply 
stating my positions.

> 
>>> I went through this driver from a high-level point of view, and I'm afraid I
>>> have quite a number of issues with this driver.
>>>
>>> One of the bigger ones is that vc8x0-ad7441.c should be implemented as
>>> a subdevice. I have two other AD drivers in my queue (adv7604 and ad9389b),
>>> so you can look at those for comparison.
>>>
>>> See: http://www.spinics.net/lists/linux-media/msg51501.html
>>
>> Oh, I understand how to write video decoder drivers. I just chose
>> specifically not to do it. This was intensional on my part. If when
>> another card comes along with an ad7441 when they are welcome to split
>> the code and/or create the subdevs. It's extra engineering today that
>> does't improve the support for the 820 card and doesn't benefit any
>> other known product. It's a feature that's exclusive to the 820.
>> Future developers are welcome to fork, slice and/or copy the code.
>> Today, the driver is tuned exactly for the card, it works very nicely
>> for end users and the code is easy to read, simple to debug and relies
>> on only a handful of v4l2 framework apis.
> 
> The problem is that it is very hard, if not impossible, to split something
> up after the fact. There are almost no drivers left that still have an
> integral i2c driver as opposed to a separate i2c driver. I only know of
> a few old ones and at least one in staging (not counting reverse
> engineered drivers such as in gspca where there is no choice).
> 
> The main advantage is that it is much easier to extend the functionality
> of your driver, particularly for devices with a lot of functionality like
> those ADV drivers.

Agreed. I also noticed that the I2C code there is currently
highly-coupled with the bridge one. For example, you're setting the DV 
timings via a direct call to vc8x0_ad7441_set_format(), which is a
method of the I2C ad7441 driver (with, btw, is not an independent driver,
but it is part of the same driver).

We only allow such kind of designs as a temporary measure, for drivers
under drivers/staging/media, where the developer is going to fix them
in order to fully use the V4L core, as highly coupled designs like that
are really hard to review, as the interactions between the drivers
aren't at the usual/expected places.

>> Today, the driver is tuned exactly for the card, it works very nicely
>> for end users and the code is easy to read, simple to debug and relies
>> on only a handful of v4l2 framework apis.

It looks easy to read for you, not for me, and likely not for the
others:

 21 files changed, 15580 insertions(+)

A 15000+ lines driver is not easy to read/understand. Having it highly
coupled and not using the core API's, but using something else makes
even harder to understand it.

>> The subdev framework (in my opinion) has become
>> unwieldily and difficult to debug, based on my recent experience
>> dealing with some cx23885 issues. I'm intensionally trying not to use
>> it.
> 
> In what respect does the subdev framework make debugging hard? I'd like to
> know about it so we can try and improve it.
> 
> The subdev framework was made to make reuse possible. If everyone refuses
> to use it, then we're back to the bad old times where you get 20
> almost-but-not-quite identical i2c driver implementations.

>> I should be clear, my comments are not mean to antagonize or inflame,
>> I'm simply pointing out that when at all possible I chose not to use
>> the subdev framework because if it's delays and difficulties when it
>> comes to debugging.
>>
>> When did the subdev framework become a mandatory requirement for any
>> driver merge?

This was always a requirement: complex drivers should be broken into
per-component drivers, in order to make easier to review and to re-use
the code. Even the drivers at Kernel 2.5.x followed that idea, and
we're working hard to simplify drivers code by making them modular
and loosely coupled.

With modular, loosely coupled drivers, a complex logic is broken into
smaller and easier to understand logic.

> It is certainly highly recommended, and in my opinion there should be
> very good reasons for not doing it. In the end it is Mauro who decides,
> although I personally would be in favor of mandating it. Not to pester
> people, but it is so much harder to split it up after the fact. Just like
> the fact that documentation is now required if a new API is added, because
> nobody ever writes documentation afterwards.

I agree. Of course exceptions might apply, if there are really very
good and well accepted reasons for not doing that.

>>> These Analog Devices chips are quite complex, and you really want to be able
>>> to reuse drivers.
>>
>> I am certainly more than willing to discuss re-use, when re-use make
>> sense. Right now we have no-practical re-use for this part to speak
>> of. The code is targeted towards the 820, in the use case that end
>> users need. If someone would like to build a ad7441 subdevice and use
>> that in their driver then they are welcome to the code. In practise,
>> sharing complex video decoders across driver designs leads to massive
>> regressions, as witnessed on the list this year.

Regressions happen even if no re-use is done: core, other parts of the
Kernel and userspace apps changes all the times, causing regressions.
That's called bitrotten[1].

[1] http://en.wikipedia.org/wiki/Bit_rot

One good example is with regards to firmware loading: today's userspace
requirement is that firmware load can't happen during driver's probe. 
All drivers that do that (almost all media drivers with firmware) are
broken (or only loads after timeout - 120 seconds?) with newer udev's,
even if you run an old kernel.

Side note: your Viewcast 0820E driver also suffers from request_firmware()
regression, as it doesn't use request_firmware_nowait() and tries to load
the firmware during device probe.

The only way to be sure that regressions aren't introduced is to run
a testbench on every new kernel release.

>> I am also aware that the cx25840 driver is complex, and the end result
>> was that driver maintainers effectively forked the cx25840 and brought
>> the codebase back into their core drivers (cx18 for example), to avoid
>> issues where regression testing was troublesome across so many cards.
> 
> I don't think the cx18 ever used the cx25840 codebase: while the cx23418
> uses a similar IP as the cx25840 there were too many differences to allow
> us (actually, I think it was me who wrote it initially for the cx18) to
> reuse the cx25840.
> 
> That's a general problem, BTW: reuse works well for the exact same chip.
> But when you get variants of the chip (or it gets integrated in another chip
> as an IP block), then at some point keeping track of those differences and
> preventing regressions becomes harder than it would be if they were done as
> separate drivers.
> 
> Where that boundary is is not always clear, and for the cx25840 we probably
> exceeded it.

Agreed.

> But sharing a subdevice driver for a single chip among different boards has
> never been much of a problem in my experience. I'm not saying we never got
> regressions, but not many and they were generally quickly caught.
> 
>> If I had the time and/or energy, we'd do the same with the cx25840.
>> Keeping complex code close to the PCI/PCIe bridge bring big dividends
>> when debugging complex problems and mitigating issues where code is
>> re-used across multiple products (growing any regression test
>> requirements).
>>
>> The entire driver was intensionally written to be self-enclosed,
>> highly portable between 2.6.3x and v3.x kernels without subdev
>> breakages and/or api changes. With a small external Makefile it even
>> builds very nicely outside of the kernel on any kernel you like, that
>> shipped in the last 2-3 years.
> 
> ??? We have the media_build system for that, so why care about older kernels?
> 
>>>
>>> Some of the other issues are:
>>>
>>> - Please use the control framework. All new drivers must use it, unless there
>>>   are very, very good reasons not to. I'm gradually converting all drivers to
>>>   the control framework, so I really don't want to introduce new drivers to
>>>   that list.
>>
>> I think the control framework is a great design, it's just too
>> difficult to debug and when I go near it - it breaks, or I spend an
>> hour trying to understand why my subdev call doesn't reach the subdev
>> device. My comments are not designed to inflame or upset you, I'm
>> simply pointing out that any work I've done recently (on two new PCIe
>> bridges - unreleased code) I've decided not to use it.
> 
> Ask me if there are problems with the control framework! I'm happy to help out.
> 
> I can't fix it, improve it, etc. if I don't here about it.
> 
> The reason why all drivers should use it is that is behaves the same for all
> drivers, and apps can start to rely on that behavior. If something doesn't work,
> and you can't figure it out, then I am more than happy to help.
> 
> But I will NACK this driver unless it is using the control framework. Otherwise
> I just have to do that later, and I really don't want to. There aren't many
> controls in this driver so it should be pretty quick to do it.
> 
> In addition, the control framework will make it easy to implement control events,
> another thing that should be rolled out to all drivers.
> 
>> Again, I specifically chosen to isolate this driver from certain key
>> areas of the (now enormous) v4l2 infrastructure.
> 
> There is a reason for that infrastructure, you know. Consistent behavior from
> the point of view of applications is the most important one. And it actually
> takes a lot of work off your hands. Again, if there are questions, then I'm
> happy to help out (and possible improve the code or documentation).
> 
>>
>>>
>>> - TRY_FMT can actually set the format, something that should never happen.
>>
>> I can check, but I think gstreamer or tvtime actually relies on that behavior.
> 
> Highly unlikely. TRY_FMT should *never* change the actual format. It's been like
> that since the very beginning.

I doubt tvtime or gstreamer would be relying on that: API is clear that try_* doesn't
change it, and drivers don't change format on try_fmt.

If your driver is working with those apps, It is probably pretty much the reverse: 
the applications are either not using try_fmt, or doing an s_fmt before streaming,
reverting any changes that might be happening with try_fmt.

>>> - Use the new DV_TIMINGS ioctls for the HDTV formats. S_FMT should not be used
>>>   to select the HDTV format!
>>
>> gstreamer on 2.6.37 and better didn't support DVTIMINGs. I would
>> certainly like to discuss adding better timing support once
>> applications are fully aware and can control the hardware using it.
>> The lack of a good timin API (and adoption by the application
>> developers) forced my hand to use S_FMT.
> 
> You said that it was in your queue for some time, so it may be that it wasn't
> available when you started out. The problem is, the API is now available, and
> if drivers do not implement it, then there is also no reason for applications
> to use it, isn't it? Chicken and egg.
> 
> I will NACK if the driver doesn't add support for it. Otherwise we end up with
> some drivers that implement it correctly, and others that use a different
> method. No application writer will ever thank us for that.
> 
> Also, I didn't go through all the hard work of designing and adding an API and
> then see it being ignored in favor of a non-standard solution.
> 
>> Right now the driver works today, on old and new systems, for hardware
>> that's shipping. It satisfies end user needs.
>>
>>>
>>> - The procfs additions seem unnecessary to me. VIDIOC_LOG_STATUS or perhaps
>>>   debugfs are probably much more suitable.
>>
>> I agree. It can probably be removed altogether.
>>
>>>
>>> - Using videobuf2 is very much recommended.
>>
>> I went with what I know to be honest. I neither agree nor disagree
>> with your comments. If videobuf2 is supported on 2.6.3x then this is
>> good news.
> 
> If you use media_build to compile your driver, then everything including vb2
> is supported from 2.6.31 onwards (the oldest kernel supported by media_build).

Btw, 2.6.31 limit is just because none cares to have it supported on older
releases. Once I needed to test a driver on 2.6.28, and it was not hard to
add the very few missing bits for that driver to work there.

>>
>>>
>>> - Please run v4l2-compliance and fix any reported issues!
>>>
>>> It's a pretty big driver, so I only looked skimmed the patch, but these are
>>> IMHO fairly major issues. As it stands it is only suitable to be merged in
>>> drivers/staging/media.
>>
>> I'm not sure I agree. I think I don't agree in general that subdev and
>> the control framework is mandatory for any driver. I think they are
>> accelerator frameworks designed to help. I my case I don't think they
>> do. So I avoided using them.
>>
>> I guess Mauro has the final decision.
> 
> Of course.

I would accept it only if you have very strong technical reasons why not
using the existing infrastructure, e. g. you need to clearly explain
what makes your driver so different that the v4l infrastructure won't work
and can't be fixed to work with?

If you're willing to change it to fulfill the requirements, the driver
could be merged into staging, if think you'll take more than 4-5 weeks
to address the pointed issues.

> BTW, I saw another thing that must be changed: you use the .ioctl file
> operation instead of unlocked_ioctl. This is no longer allowed for new
> drivers since the removal of the BKL. The v4l2 core implementation of
> .ioctl attempts to simulate the old BKL and is very inefficient, in
> particular for drivers like this that do not use struct v4l2_device.
> If you have multiple ViewCast cards, then all v4l2 ioctls will go through
> a single V4L2 core lock, leading to substantial latencies.
> 
> See also the comment in v4l2_ioctl() in v4l2_dev.c.
> 
> New drivers must use unlocked_ioctl. I am in the (very slow, but steady)
> process of fixing any old drivers that still use .ioctl and once all are
> converted .ioctl will be removed altogether.

Regards,
Mauro

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  

On Mon, Aug 13, 2012 at 1:36 PM, Mauro Carvalho Chehab
<mchehab@redhat.com> wrote:
> Em 13-08-2012 12:49, Hans Verkuil escreveu:
>> On Mon August 13 2012 16:46:45 Steven Toth wrote:
>>> Hans,
>>>
>>> Thanks for your feedback.
>>>
>>> Oh dear. I don't think you're going to like my response, but I think
>>> we know each other well enough to realize that neither of us are
>>> trying to antagonize or upset either other. We're simply stating our
>>> positions. Please read on.
>>
>> I didn't think you'd like my response either :-)
>
> You probably won't like my answer too, yet I'm also simply
> stating my positions.

Hans / Mauro, thank you for your comments and review, very good
feedback and technical discussion. Truly, thank you. :)

While I don't necessarily agree with Mauro that adoption of subdev is
"MANDATORY" (in the larger sense of the kernel driver development -
and common practices throughout the entire source base), I do hear and
value your comments and concerns from a peer review perspective.

1) A handful of simple improvements have been suggested, Eg.
ioctl_unlocked, double-checking v4l2-compliance, try_fmt, /proc
removal, firmware loading etc

Ack. I have no objections. Items like this are fairly trivial, easy to
address, I can absorb this and provide new patches quickly and easily.
I'll go back over the detailed comments this weekend and prepare
additional patches (and retest).

2) ... and some larger discussion items have been raised, Eg.
Absorbing more of the V4L2 kernel infrastructure into the vc8x0 driver
vs a fairly self-contained driver with very limited opportunities for
future breakage.

Are you really willing to say that all drivers, with unique and new
pieces of silicon, need to be split out into independent modules,
adopting a subdev type interfaces or mainline merge is refused? It's
not that I'm asking you to merge duplicate functionality, duplicate
driver code, replicating functionality for new hardware or for an
existing modules (tuner/demod/whatever). (Like has already happened in
the past - 18271 for example).

If the answer is Yes, then my second questions is:

Assuming the comments / issues mentioned in #1 are addressed, are you
really willing to stand up in front of the world-wide Kernel
development community and justify why a driver that passes user-facing
v4l2-compliance tests, is fairly clean, passes 99% of the reasonable
checkpatch rules, is fully operational, cannot be merged? Really? Is
this truly an illegal or inappropriate driver implementation that
would prohibit mainline merge?

The ViewCast 820 is a (circa) $1800 video capture card. It's not the
kind of hardware that everyone has laying around for regression
testing purposes. If I 1) split this up and people begin to absorb
ad7441 functionality into other designs, and start patching it and 2)
adopt the subdev framework for that matter... then nobody is able to
regression test the impact to the 820. That puts an incredible amount
of burden on me. I'm attempting to mitigate all of this risk, but also
provide a GPL driver so everyone can benefit - without creating a
future maintenance / regression problem, by relying on subsystems the
driver simply doesn't need.

As always, I do welcome and appreciate your comments and thoughts, no
flames from me. I do find the 'MANDATORY' comments worthy of
discussion, or perhaps I've miss-understood something.

Regards,

- Steve
  

On Tue August 14 2012 17:07:55 Steven Toth wrote:
> On Mon, Aug 13, 2012 at 1:36 PM, Mauro Carvalho Chehab
> <mchehab@redhat.com> wrote:
> > Em 13-08-2012 12:49, Hans Verkuil escreveu:
> >> On Mon August 13 2012 16:46:45 Steven Toth wrote:
> >>> Hans,
> >>>
> >>> Thanks for your feedback.
> >>>
> >>> Oh dear. I don't think you're going to like my response, but I think
> >>> we know each other well enough to realize that neither of us are
> >>> trying to antagonize or upset either other. We're simply stating our
> >>> positions. Please read on.
> >>
> >> I didn't think you'd like my response either :-)
> >
> > You probably won't like my answer too, yet I'm also simply
> > stating my positions.
> 
> Hans / Mauro, thank you for your comments and review, very good
> feedback and technical discussion. Truly, thank you. :)
> 
> While I don't necessarily agree with Mauro that adoption of subdev is
> "MANDATORY" (in the larger sense of the kernel driver development -
> and common practices throughout the entire source base), I do hear and
> value your comments and concerns from a peer review perspective.

You're awfully polite for someone whose code has been shot down :-)
Don't worry, I'll buy you a beer in San Diego to soften the pain (or to
drown your sorrows!).

> 1) A handful of simple improvements have been suggested, Eg.
> ioctl_unlocked, double-checking v4l2-compliance, try_fmt, /proc
> removal, firmware loading etc
> 
> Ack. I have no objections. Items like this are fairly trivial, easy to
> address, I can absorb this and provide new patches quickly and easily.
> I'll go back over the detailed comments this weekend and prepare
> additional patches (and retest).

Note that the v4l2-compliance tests are generally more strict than the
spec itself. For example, it assumes that the control framework is used,
that control events are implemented, and that vb2 is used.

Take a look at vivi.c: it implements all the latest infrastructure and it
is now actually a pretty good example of how things should work.

It's also one of the few drivers that passes all v4l2-compliance tests.

The only ioctls that aren't covered yet by v4l2-compliance are:

           VIDIOC_CROPCAP, VIDIOC_G/S_CROP, VIDIOC_G/S_SELECTION
           VIDIOC_S_FBUF/OVERLAY
           VIDIOC_(TRY_)ENCODER_CMD
           VIDIOC_(TRY_)DECODER_CMD
           VIDIOC_G_ENC_INDEX
           VIDIOC_QBUF/DQBUF/QUERYBUF/PREPARE_BUFS
           VIDIOC_STREAMON/OFF

So basically cropping, compression encoder/decoder control and actual
streaming. And the subdev and media API is also not tested, although
those might be beyond the scope of this utility anyway.

Everything else is now tested fairly exhaustively.

> 2) ... and some larger discussion items have been raised, Eg.
> Absorbing more of the V4L2 kernel infrastructure into the vc8x0 driver
> vs a fairly self-contained driver with very limited opportunities for
> future breakage.
> 
> Are you really willing to say that all drivers, with unique and new
> pieces of silicon, need to be split out into independent modules,
> adopting a subdev type interfaces or mainline merge is refused? It's
> not that I'm asking you to merge duplicate functionality, duplicate
> driver code, replicating functionality for new hardware or for an
> existing modules (tuner/demod/whatever). (Like has already happened in
> the past - 18271 for example).

Speaking for myself, I would probably NACK it, yes. I would hate to do it,
but there are IMHO good technical reasons why the ad7441 code should be
implemented as a subdev driver.

> If the answer is Yes, then my second questions is:
> 
> Assuming the comments / issues mentioned in #1 are addressed, are you
> really willing to stand up in front of the world-wide Kernel
> development community and justify why a driver that passes user-facing
> v4l2-compliance tests, is fairly clean, passes 99% of the reasonable
> checkpatch rules, is fully operational, cannot be merged? Really? Is
> this truly an illegal or inappropriate driver implementation that
> would prohibit mainline merge?

Yes. Currently nobody else uses the ad7441 but the viewcast driver. So
splitting it up really shouldn't be too much of a problem: you don't have
to take care of anyone else, and it only has to support the functionality
that you need right now. And as long as nobody else uses that driver it
shouldn't make a difference to you maintenance-wise.

But *if* someone else comes along then that will help them enormously if
an ad7441 driver already exists. We definitely do not want to have duplicate
drivers in the kernel for i2c devices, so either they or you would have to
split up the ad7441 driver from the ViewCast driver, and what are the chances
of that ever happening? Slim to none.

You just want to get your driver merged, which is perfectly understandable,
but I also want to ensure that whatever gets merged can also be reused by
others, where applicable.

In addition to that I have to say that I have been working with Analog Devices
i2c receivers and transmitters for the past 4-5 years, and these things are
complex. I consider it very unlikely that your ad7441 driver covers the full
functionality of the ad7441. By implementing it as a separate driver it will
be much easier for others to work on it and improve it. Yes, that might
require you to do the occasional testing, but hopefully that will improve the
functionality of the ViewCast driver as well by e.g. supporting more formats,
have better colorspace handling, or whatever.

Also note that the Analog Devices receivers/transmitters are fairly popular,
particularly within the embedded hardware world. So it wouldn't surprise me
at all if other products will appear that want to use it.

One other difference with such subdev drivers is that they can be in the
kernel, while the actual V4L2 driver for an embedded product isn't.

Typically on embedded systems the platform V4L2 driver is too product-specific
to ever be considered for upstreaming (and it is usually fairly trivial as
well). But being able to reuse an ad7441-like driver saves companies a huge
amount of time. The adv7604 and ad9883b drivers that are in my queue are like
that: the actual V4L2 driver that uses them won't be upstreamed, but we and
other companies will reuse the subdev drivers. Even better, with a bit of
luck Analog Devices themselves might get involved and start making their own
drivers.

The complexity of V4L2 drivers is in the DMA engine (for embedded devices
this is often provided by the SoC vendor) and in the video receivers,
transmitters and/or sensors (usually i2c devices). More advanced SoCs also
have video processing capabilities, but that too is/should be provided by
the SoC vendor. So as an embedded product developer you generally have the
code for the DMA engine/video processing from your SoC vendor (and V4L2 is
making steady progress there), and that leaves the other complex part: the
i2c receiver/transmitter/sensor. So having that available for reuse in the
kernel makes a big difference in development time. Particularly if you also
want to support analog video input or output (analog takes 10 times as much
development effort as digital does).

BTW, we are talking about the adv7441a, right?
See here: http://ez.analog.com/docs/DOC-1546

There is also a chip called ad7441, but that seems to be something else.
AD has the annoying habit of renaming chips, but at least they've started
making their datasheets freely available, which is very good news for linux.

> The ViewCast 820 is a (circa) $1800 video capture card. It's not the
> kind of hardware that everyone has laying around for regression
> testing purposes. If I 1) split this up and people begin to absorb
> ad7441 functionality into other designs, and start patching it and 2)
> adopt the subdev framework for that matter... then nobody is able to
> regression test the impact to the 820. That puts an incredible amount
> of burden on me. I'm attempting to mitigate all of this risk, but also
> provide a GPL driver so everyone can benefit - without creating a
> future maintenance / regression problem, by relying on subsystems the
> driver simply doesn't need.

What you are basically saying is that you don't want to split it up because
if you do, then other people might reuse the code, change it, and might cause
you a lot of work.

What I am saying is that if you split it up, then other people might reuse it,
improve it and with a relatively small amount of work improve the ViewCast
820 support as well.

I suspect your view of the amount of work it might cost you to test changes
from other people is too pessimistic. It's based on your experiences with the
cx25840, but from my perspective the cx25840 is the exception, not the rule.

And the cx25840 provides a good lesson how it may be counterproductive trying
to support multiple variants of a device in one driver. It only works if the
differences are really small, otherwise it is probably better to make separate
drivers, or make separate drivers, but have them share some common code. It's
something I'm considering for the adv drivers as I have two more drivers in
my queue that are similar, but not identical, to the adv7604 and ad9389b.

On the one hand, there is just too much identical code to justify two fully
independent drivers, but on the other hand there are too many differences as
well. I think it is possible to refactor out clearly common parts that do not
directly touch on registers. I don't know for certain yet, though.

> As always, I do welcome and appreciate your comments and thoughts, no
> flames from me. I do find the 'MANDATORY' comments worthy of
> discussion, or perhaps I've miss-understood something.

No, you understood it perfectly :-)

Regards,

	Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  

>> While I don't necessarily agree with Mauro that adoption of subdev is
>> "MANDATORY" (in the larger sense of the kernel driver development -
>> and common practices throughout the entire source base), I do hear and
>> value your comments and concerns from a peer review perspective.
>
> You're awfully polite for someone whose code has been shot down :-)
> Don't worry, I'll buy you a beer in San Diego to soften the pain (or to
> drown your sorrows!).

:)

It benefits everyone when rationale discussion can be had, even when
points of view differ. The alternative is we shout at each other over
email. Shouting gets old very quickly and never accomplishes anything.

>
>> 1) A handful of simple improvements have been suggested, Eg.
>> ioctl_unlocked, double-checking v4l2-compliance, try_fmt, /proc
>> removal, firmware loading etc
>>
>> Ack. I have no objections. Items like this are fairly trivial, easy to
>> address, I can absorb this and provide new patches quickly and easily.
>> I'll go back over the detailed comments this weekend and prepare
>> additional patches (and retest).
>
> Note that the v4l2-compliance tests are generally more strict than the
> spec itself. For example, it assumes that the control framework is used,
> that control events are implemented, and that vb2 is used.

I made some patches to the current tree to fixup some of the earlier
comments, firmware loading via nowait, unlocked_ioctl2, /proc removal.
I also ran the compliance testing tool. After realizing my version of
v4l2-compliance was a little out of date - and building fresh from
v4l2-utils, it turned out some highly useful information about the
driver.

So, I've ran v4l2-compliance and it pointed out a few things that I've
fixed, but it also does a few things that (for some reason) I can't
seem to catch. One particular test is on (iirc) s_fmt. It attempts to
set ATSC but by ioctl callback never receives ATSC in the norm/id arg,
it actually receives 0x0. This feels more like a bug in the test.
Either way, I have some if (std & ATSC) return -EINVAL, but it still
appears to fail the test.

I see some tests which report failure (testing videobuf) but given
that I essentially pass the ioctl directly into the videbug core, very
much like every oher driver I've ever done, it's probably either a
quirk of the tool, or something inside videobuf core itself that needs
some adjustment. (userptr/mmap for capture or output buffers related
test).

In summary, the v4l2-compliance tool has pointed out a few things that
were worth fixing for sure, and thus I've fixed. It it also feels like
the tool itself is still evolving. When I get a moment I'll run the
compliance tool and paste the results here for comment.

I'd welcome your feedback on the compliance feedback.

>
> Take a look at vivi.c: it implements all the latest infrastructure and it
> is now actually a pretty good example of how things should work.

Noted, that's a useful reference point.

>
> It's also one of the few drivers that passes all v4l2-compliance tests.
>
> The only ioctls that aren't covered yet by v4l2-compliance are:
>
>            VIDIOC_CROPCAP, VIDIOC_G/S_CROP, VIDIOC_G/S_SELECTION
>            VIDIOC_S_FBUF/OVERLAY
>            VIDIOC_(TRY_)ENCODER_CMD
>            VIDIOC_(TRY_)DECODER_CMD
>            VIDIOC_G_ENC_INDEX
>            VIDIOC_QBUF/DQBUF/QUERYBUF/PREPARE_BUFS
>            VIDIOC_STREAMON/OFF
>
> So basically cropping, compression encoder/decoder control and actual
> streaming. And the subdev and media API is also not tested, although
> those might be beyond the scope of this utility anyway.
>
> Everything else is now tested fairly exhaustively.
>
>> 2) ... and some larger discussion items have been raised, Eg.
>> Absorbing more of the V4L2 kernel infrastructure into the vc8x0 driver
>> vs a fairly self-contained driver with very limited opportunities for
>> future breakage.
>>
>> Are you really willing to say that all drivers, with unique and new
>> pieces of silicon, need to be split out into independent modules,
>> adopting a subdev type interfaces or mainline merge is refused? It's
>> not that I'm asking you to merge duplicate functionality, duplicate
>> driver code, replicating functionality for new hardware or for an
>> existing modules (tuner/demod/whatever). (Like has already happened in
>> the past - 18271 for example).
>
> Speaking for myself, I would probably NACK it, yes. I would hate to do it,
> but there are IMHO good technical reasons why the ad7441 code should be
> implemented as a subdev driver.

I hear you. In the spirit of co-operation I'll take a shot at turning
ad7441 into a subdev and see what odd problems shake out of the
process. I should be clear that the resulting subdevice will likely be
very 820 specific in terms of configuration, but it's a reasonable
cut-point.

>
>> If the answer is Yes, then my second questions is:
>>
>> Assuming the comments / issues mentioned in #1 are addressed, are you
>> really willing to stand up in front of the world-wide Kernel
>> development community and justify why a driver that passes user-facing
>> v4l2-compliance tests, is fairly clean, passes 99% of the reasonable
>> checkpatch rules, is fully operational, cannot be merged? Really? Is
>> this truly an illegal or inappropriate driver implementation that
>> would prohibit mainline merge?
>
> Yes. Currently nobody else uses the ad7441 but the viewcast driver. So
> splitting it up really shouldn't be too much of a problem: you don't have
> to take care of anyone else, and it only has to support the functionality
> that you need right now. And as long as nobody else uses that driver it
> shouldn't make a difference to you maintenance-wise.

Fair enough.

>
> But *if* someone else comes along then that will help them enormously if
> an ad7441 driver already exists. We definitely do not want to have duplicate
> drivers in the kernel for i2c devices, so either they or you would have to
> split up the ad7441 driver from the ViewCast driver, and what are the chances
> of that ever happening? Slim to none.
>
> You just want to get your driver merged, which is perfectly understandable,
> but I also want to ensure that whatever gets merged can also be reused by
> others, where applicable.
>
> In addition to that I have to say that I have been working with Analog Devices
> i2c receivers and transmitters for the past 4-5 years, and these things are
> complex. I consider it very unlikely that your ad7441 driver covers the full
> functionality of the ad7441. By implementing it as a separate driver it will

Yes.

> be much easier for others to work on it and improve it. Yes, that might
> require you to do the occasional testing, but hopefully that will improve the
> functionality of the ViewCast driver as well by e.g. supporting more formats,
> have better colorspace handling, or whatever.

<snip>

> Also note that the Analog Devices receivers/transmitters are fairly popular,
> particularly within the embedded hardware world. So it wouldn't surprise me
> at all if other products will appear that want to use it.

(7441 commentary removed)

Agreed.

>
> BTW, we are talking about the adv7441a, right?
> See here: http://ez.analog.com/docs/DOC-1546

Yes.

>
> There is also a chip called ad7441, but that seems to be something else.
> AD has the annoying habit of renaming chips, but at least they've started
> making their datasheets freely available, which is very good news for linux.

Force of habit on my part, it's the adv7441a.

>
>> The ViewCast 820 is a (circa) $1800 video capture card. It's not the
>> kind of hardware that everyone has laying around for regression
>> testing purposes. If I 1) split this up and people begin to absorb
>> ad7441 functionality into other designs, and start patching it and 2)
>> adopt the subdev framework for that matter... then nobody is able to
>> regression test the impact to the 820. That puts an incredible amount
>> of burden on me. I'm attempting to mitigate all of this risk, but also
>> provide a GPL driver so everyone can benefit - without creating a
>> future maintenance / regression problem, by relying on subsystems the
>> driver simply doesn't need.
>
> What you are basically saying is that you don't want to split it up because
> if you do, then other people might reuse the code, change it, and might cause
> you a lot of work.

Yes.

>
> What I am saying is that if you split it up, then other people might reuse it,
> improve it and with a relatively small amount of work improve the ViewCast
> 820 support as well.

... and it would require regression testing on every change.

>
> I suspect your view of the amount of work it might cost you to test changes
> from other people is too pessimistic. It's based on your experiences with the
> cx25840, but from my perspective the cx25840 is the exception, not the rule.

My comments are largely influenced by the 25840, and we both agree
we've stretched that driver too far and too thinly, beyond reasonable
use - to the point where we're causing regressions even with small
amounts of rework. In hindsight we all know not to let that happen
again - but these things have a habit of slowly creeping up on you and
they're a problem before you know it.

My goal is/was to head that off right at the outset, but without
limiting anyone else by my actions. I've implemented just enough of
the 7441 for what the 820 needs. A full blown subdev implementation
(for all features) is beyond the 820 needs, out of scope. I'll take a
shot at converting the limited functionality into a subdev as-is,
let's see. If you accept that it's likely to be fairly 820 specific
(not unreasonable to begin with) then that's a reasonable compromise.

>
> And the cx25840 provides a good lesson how it may be counterproductive trying
> to support multiple variants of a device in one driver. It only works if the
> differences are really small, otherwise it is probably better to make separate
> drivers, or make separate drivers, but have them share some common code. It's
> something I'm considering for the adv drivers as I have two more drivers in
> my queue that are similar, but not identical, to the adv7604 and ad9389b.

Agreed.

>
> On the one hand, there is just too much identical code to justify two fully
> independent drivers, but on the other hand there are too many differences as
> well. I think it is possible to refactor out clearly common parts that do not
> directly touch on registers. I don't know for certain yet, though.

It's tough. I air on the side of keeping the code reasonable and
readable, easy to digest and support - even if it means small amounts
of duplication. If the goal of LinuxTV is to welcome less experience
video kernel developers then it's off-putting to have very abstract,
scientific driver creations that can only be maintained by one or two
people.

I don't know the 7604 or the 9389 but I'd suggest simple is better
(for the rest of the group), unless they're 99% identical (unlikely).

>
>> As always, I do welcome and appreciate your comments and thoughts, no
>> flames from me. I do find the 'MANDATORY' comments worthy of
>> discussion, or perhaps I've miss-understood something.
>
> No, you understood it perfectly :-)

Regards,
  

On Thu August 16 2012 15:27:51 Steven Toth wrote:
> >> While I don't necessarily agree with Mauro that adoption of subdev is
> >> "MANDATORY" (in the larger sense of the kernel driver development -
> >> and common practices throughout the entire source base), I do hear and
> >> value your comments and concerns from a peer review perspective.
> >
> > You're awfully polite for someone whose code has been shot down :-)
> > Don't worry, I'll buy you a beer in San Diego to soften the pain (or to
> > drown your sorrows!).
> 
> :)
> 
> It benefits everyone when rationale discussion can be had, even when
> points of view differ. The alternative is we shout at each other over
> email. Shouting gets old very quickly and never accomplishes anything.
> 
> >
> >> 1) A handful of simple improvements have been suggested, Eg.
> >> ioctl_unlocked, double-checking v4l2-compliance, try_fmt, /proc
> >> removal, firmware loading etc
> >>
> >> Ack. I have no objections. Items like this are fairly trivial, easy to
> >> address, I can absorb this and provide new patches quickly and easily.
> >> I'll go back over the detailed comments this weekend and prepare
> >> additional patches (and retest).
> >
> > Note that the v4l2-compliance tests are generally more strict than the
> > spec itself. For example, it assumes that the control framework is used,
> > that control events are implemented, and that vb2 is used.
> 
> I made some patches to the current tree to fixup some of the earlier
> comments, firmware loading via nowait, unlocked_ioctl2, /proc removal.
> I also ran the compliance testing tool. After realizing my version of
> v4l2-compliance was a little out of date - and building fresh from
> v4l2-utils, it turned out some highly useful information about the
> driver.
> 
> So, I've ran v4l2-compliance and it pointed out a few things that I've
> fixed, but it also does a few things that (for some reason) I can't
> seem to catch. One particular test is on (iirc) s_fmt. It attempts to
> set ATSC but by ioctl callback never receives ATSC in the norm/id arg,
> it actually receives 0x0. This feels more like a bug in the test.
> Either way, I have some if (std & ATSC) return -EINVAL, but it still
> appears to fail the test.

I think it might be because vdev->tvnorms isn't set for the video node.
It's set for the VBI node, though. If tvnorms is 0, then ENUMSTD will
probably return an empty list, and that might be the cause of the ATSC
test. I also see that current_norm is used: don't do that. Instead store
the current standard yourself and return it in g_std. I'm slowly phasing
out current_norm because 1) it's ugly and 2) it doesn't work if you have
both a vbi and a video node.

> I see some tests which report failure (testing videobuf) but given
> that I essentially pass the ioctl directly into the videbug core, very
> much like every oher driver I've ever done, it's probably either a
> quirk of the tool, or something inside videobuf core itself that needs
> some adjustment. (userptr/mmap for capture or output buffers related
> test).

videobuf does not follow the spec in several areas (most notably and extremely
annoyingly it does not support calling REQBUFS with a count of 0). So any
driver that uses videobuf will fail a number of tests in v4l2-compliance.

These problems are fixed in vb2, which is why I strongly recommend its use in
new drivers. In addition, work on the new DMABUF mechanism for sharing buffers
between v4l2 and drm will only be implemented in vb2.

> In summary, the v4l2-compliance tool has pointed out a few things that
> were worth fixing for sure, and thus I've fixed. It it also feels like
> the tool itself is still evolving. When I get a moment I'll run the
> compliance tool and paste the results here for comment.

It is steadily being improved, that's correct.

BTW, don't forget to also run it for the vbi node (option '-V /dev/vbiX').

Feedback on the tool is very much appreciated! E.g. how to improve it, whether
there are missing, confusing or incorrect tests, etc.

> I'd welcome your feedback on the compliance feedback.

No problem. One set of errors that v4l2-compliance produces is that it fails
if you can change VBI formats using a video node or vice versa. I want to
address that in the V4L2 core rather than having to fix all drivers. It's one
of the topics in the V4L2 ambiguities list for the upcoming workshop.

> > Take a look at vivi.c: it implements all the latest infrastructure and it
> > is now actually a pretty good example of how things should work.
> 
> Noted, that's a useful reference point.
> 
> >
> > It's also one of the few drivers that passes all v4l2-compliance tests.
> >
> > The only ioctls that aren't covered yet by v4l2-compliance are:
> >
> >            VIDIOC_CROPCAP, VIDIOC_G/S_CROP, VIDIOC_G/S_SELECTION
> >            VIDIOC_S_FBUF/OVERLAY
> >            VIDIOC_(TRY_)ENCODER_CMD
> >            VIDIOC_(TRY_)DECODER_CMD
> >            VIDIOC_G_ENC_INDEX
> >            VIDIOC_QBUF/DQBUF/QUERYBUF/PREPARE_BUFS
> >            VIDIOC_STREAMON/OFF
> >
> > So basically cropping, compression encoder/decoder control and actual
> > streaming. And the subdev and media API is also not tested, although
> > those might be beyond the scope of this utility anyway.
> >
> > Everything else is now tested fairly exhaustively.
> >
> >> 2) ... and some larger discussion items have been raised, Eg.
> >> Absorbing more of the V4L2 kernel infrastructure into the vc8x0 driver
> >> vs a fairly self-contained driver with very limited opportunities for
> >> future breakage.
> >>
> >> Are you really willing to say that all drivers, with unique and new
> >> pieces of silicon, need to be split out into independent modules,
> >> adopting a subdev type interfaces or mainline merge is refused? It's
> >> not that I'm asking you to merge duplicate functionality, duplicate
> >> driver code, replicating functionality for new hardware or for an
> >> existing modules (tuner/demod/whatever). (Like has already happened in
> >> the past - 18271 for example).
> >
> > Speaking for myself, I would probably NACK it, yes. I would hate to do it,
> > but there are IMHO good technical reasons why the ad7441 code should be
> > implemented as a subdev driver.
> 
> I hear you. In the spirit of co-operation I'll take a shot at turning
> ad7441 into a subdev and see what odd problems shake out of the
> process. I should be clear that the resulting subdevice will likely be
> very 820 specific in terms of configuration, but it's a reasonable
> cut-point.

That's a perfectly valid approach. Frankly, I think it is counter productive
to try and make a more general implementation. You only end up with code that
you can't test on your hardware. Such code generally suffers badly from bitrot
over time.

> >
> >> If the answer is Yes, then my second questions is:
> >>
> >> Assuming the comments / issues mentioned in #1 are addressed, are you
> >> really willing to stand up in front of the world-wide Kernel
> >> development community and justify why a driver that passes user-facing
> >> v4l2-compliance tests, is fairly clean, passes 99% of the reasonable
> >> checkpatch rules, is fully operational, cannot be merged? Really? Is
> >> this truly an illegal or inappropriate driver implementation that
> >> would prohibit mainline merge?
> >
> > Yes. Currently nobody else uses the ad7441 but the viewcast driver. So
> > splitting it up really shouldn't be too much of a problem: you don't have
> > to take care of anyone else, and it only has to support the functionality
> > that you need right now. And as long as nobody else uses that driver it
> > shouldn't make a difference to you maintenance-wise.
> 
> Fair enough.
> 
> >
> > But *if* someone else comes along then that will help them enormously if
> > an ad7441 driver already exists. We definitely do not want to have duplicate
> > drivers in the kernel for i2c devices, so either they or you would have to
> > split up the ad7441 driver from the ViewCast driver, and what are the chances
> > of that ever happening? Slim to none.
> >
> > You just want to get your driver merged, which is perfectly understandable,
> > but I also want to ensure that whatever gets merged can also be reused by
> > others, where applicable.
> >
> > In addition to that I have to say that I have been working with Analog Devices
> > i2c receivers and transmitters for the past 4-5 years, and these things are
> > complex. I consider it very unlikely that your ad7441 driver covers the full
> > functionality of the ad7441. By implementing it as a separate driver it will
> 
> Yes.
> 
> > be much easier for others to work on it and improve it. Yes, that might
> > require you to do the occasional testing, but hopefully that will improve the
> > functionality of the ViewCast driver as well by e.g. supporting more formats,
> > have better colorspace handling, or whatever.
> 
> <snip>
> 
> > Also note that the Analog Devices receivers/transmitters are fairly popular,
> > particularly within the embedded hardware world. So it wouldn't surprise me
> > at all if other products will appear that want to use it.
> 
> (7441 commentary removed)
> 
> Agreed.
> 
> >
> > BTW, we are talking about the adv7441a, right?
> > See here: http://ez.analog.com/docs/DOC-1546
> 
> Yes.
> 
> >
> > There is also a chip called ad7441, but that seems to be something else.
> > AD has the annoying habit of renaming chips, but at least they've started
> > making their datasheets freely available, which is very good news for linux.
> 
> Force of habit on my part, it's the adv7441a.

It would be nice if you can do a search and replace so that the correct name
is used in the sources.

And a comment in the source with the URL of the datasheets as I gave above
is very useful as well. Analog Devices did/are doing a good job when it comes
to publishing datasheets.

> >
> >> The ViewCast 820 is a (circa) $1800 video capture card. It's not the
> >> kind of hardware that everyone has laying around for regression
> >> testing purposes. If I 1) split this up and people begin to absorb
> >> ad7441 functionality into other designs, and start patching it and 2)
> >> adopt the subdev framework for that matter... then nobody is able to
> >> regression test the impact to the 820. That puts an incredible amount
> >> of burden on me. I'm attempting to mitigate all of this risk, but also
> >> provide a GPL driver so everyone can benefit - without creating a
> >> future maintenance / regression problem, by relying on subsystems the
> >> driver simply doesn't need.
> >
> > What you are basically saying is that you don't want to split it up because
> > if you do, then other people might reuse the code, change it, and might cause
> > you a lot of work.
> 
> Yes.
> 
> >
> > What I am saying is that if you split it up, then other people might reuse it,
> > improve it and with a relatively small amount of work improve the ViewCast
> > 820 support as well.
> 
> ... and it would require regression testing on every change.
> 
> >
> > I suspect your view of the amount of work it might cost you to test changes
> > from other people is too pessimistic. It's based on your experiences with the
> > cx25840, but from my perspective the cx25840 is the exception, not the rule.
> 
> My comments are largely influenced by the 25840, and we both agree
> we've stretched that driver too far and too thinly, beyond reasonable
> use - to the point where we're causing regressions even with small
> amounts of rework. In hindsight we all know not to let that happen
> again - but these things have a habit of slowly creeping up on you and
> they're a problem before you know it.
> 
> My goal is/was to head that off right at the outset, but without
> limiting anyone else by my actions. I've implemented just enough of
> the 7441 for what the 820 needs. A full blown subdev implementation
> (for all features) is beyond the 820 needs, out of scope. I'll take a
> shot at converting the limited functionality into a subdev as-is,
> let's see. If you accept that it's likely to be fairly 820 specific
> (not unreasonable to begin with) then that's a reasonable compromise.

As I mentioned above, that's a perfectly reasonable approach, and actually
one that I would recommend regardless.

I have a few remarks with regards to converting the 7441 driver to a subdev
driver: I recommend doing this last and fixing all other items first. The
reason is that you probably need two additional pieces of code. The first
is some additional API support to handle HDMI/DVI/etc. connectors correctly,
esp. with respect to hotplug support and EDID support. I'll post what is
hopefully the final pull request for this infrastructure tomorrow. See in the
meantime my hdmi3 tree:

http://git.linuxtv.org/hverkuil/media_tree.git/shortlog/refs/heads/hdmi3

The other part is that there is no easy way at the moment to have a subdev
driver notify the bridge driver when a control changes value. This is relevant
for these adv drivers since the new API adds read-only status controls that
are updated whenever e.g. RX Sense detects power on an input. For embedded systems
you can simply receive a control event whenever those read-only controls change
value, but for a bridge driver there is no simple method to get that event.

It may not be a big deal for your driver since this tends to be more important
for transmitters, but it is something I have to fix regardless.

My plan is that the bridge driver's notify function (part of struct v4l2_device)
is called, but I need to do some refactoring in the event code to make that
possible. I want to work on that in the very near future since others need it
as well. The workaround at the moment would be to add a manual notify call
after updating the control's value.

> > And the cx25840 provides a good lesson how it may be counterproductive trying
> > to support multiple variants of a device in one driver. It only works if the
> > differences are really small, otherwise it is probably better to make separate
> > drivers, or make separate drivers, but have them share some common code. It's
> > something I'm considering for the adv drivers as I have two more drivers in
> > my queue that are similar, but not identical, to the adv7604 and ad9389b.
> 
> Agreed.
> 
> >
> > On the one hand, there is just too much identical code to justify two fully
> > independent drivers, but on the other hand there are too many differences as
> > well. I think it is possible to refactor out clearly common parts that do not
> > directly touch on registers. I don't know for certain yet, though.
> 
> It's tough. I air on the side of keeping the code reasonable and
> readable, easy to digest and support - even if it means small amounts
> of duplication. If the goal of LinuxTV is to welcome less experience
> video kernel developers then it's off-putting to have very abstract,
> scientific driver creations that can only be maintained by one or two
> people.
> 
> I don't know the 7604 or the 9389 but I'd suggest simple is better
> (for the rest of the group), unless they're 99% identical (unlikely).

Regards,

	Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  

>> So, I've ran v4l2-compliance and it pointed out a few things that I've
>> fixed, but it also does a few things that (for some reason) I can't
>> seem to catch. One particular test is on (iirc) s_fmt. It attempts to
>> set ATSC but by ioctl callback never receives ATSC in the norm/id arg,
>> it actually receives 0x0. This feels more like a bug in the test.
>> Either way, I have some if (std & ATSC) return -EINVAL, but it still
>> appears to fail the test.

Oddly enough. If I set tvnorms to something valid, then compliance
passes but gstreamer
fails to run, looks like some kind of confusion about either the
current established
norm, or a failure to establish a norm.

For the time being I've set tvnorms to 0 (with a comment) and removed
current_norm.

>
> I think it might be because vdev->tvnorms isn't set for the video node.
> It's set for the VBI node, though. If tvnorms is 0, then ENUMSTD will
> probably return an empty list, and that might be the cause of the ATSC
> test. I also see that current_norm is used: don't do that. Instead store
> the current standard yourself and return it in g_std. I'm slowly phasing
> out current_norm because 1) it's ugly and 2) it doesn't work if you have
> both a vbi and a video node.

Done.

>
>> I see some tests which report failure (testing videobuf) but given
>> that I essentially pass the ioctl directly into the videbug core, very
>> much like every oher driver I've ever done, it's probably either a
>> quirk of the tool, or something inside videobuf core itself that needs
>> some adjustment. (userptr/mmap for capture or output buffers related
>> test).
>
> videobuf does not follow the spec in several areas (most notably and extremely
> annoyingly it does not support calling REQBUFS with a count of 0). So any
> driver that uses videobuf will fail a number of tests in v4l2-compliance.

Compliance as of today (with changes), now gives:

"Buffer ioctls:
		fail: v4l2-test-buffers.cpp(76): can_stream && !mmap_valid && !userptr_valid
	test VIDIOC_REQBUFS/CREATE_BUFS: FAIL
	test read/write: OK"


>
> These problems are fixed in vb2, which is why I strongly recommend its use in
> new drivers. In addition, work on the new DMABUF mechanism for sharing buffers
> between v4l2 and drm will only be implemented in vb2.
>
>> In summary, the v4l2-compliance tool has pointed out a few things that
>> were worth fixing for sure, and thus I've fixed. It it also feels like
>> the tool itself is still evolving. When I get a moment I'll run the
>> compliance tool and paste the results here for comment.
>
> It is steadily being improved, that's correct.
>
> BTW, don't forget to also run it for the vbi node (option '-V /dev/vbiX').

It's got a couple more failures I need to take care of, nothing huge.

>
> Feedback on the tool is very much appreciated! E.g. how to improve it, whether
> there are missing, confusing or incorrect tests, etc.
>
>> I'd welcome your feedback on the compliance feedback.
>
> No problem. One set of errors that v4l2-compliance produces is that it fails
> if you can change VBI formats using a video node or vice versa. I want to
> address that in the V4L2 core rather than having to fix all drivers. It's one
> of the topics in the V4L2 ambiguities list for the upcoming workshop.
>
>> > Take a look at vivi.c: it implements all the latest infrastructure and it
>> > is now actually a pretty good example of how things should work.
>>
>> Noted, that's a useful reference point.
>>
>> >
>> > It's also one of the few drivers that passes all v4l2-compliance tests.
>> >
>> > The only ioctls that aren't covered yet by v4l2-compliance are:
>> >
>> >            VIDIOC_CROPCAP, VIDIOC_G/S_CROP, VIDIOC_G/S_SELECTION
>> >            VIDIOC_S_FBUF/OVERLAY
>> >            VIDIOC_(TRY_)ENCODER_CMD
>> >            VIDIOC_(TRY_)DECODER_CMD
>> >            VIDIOC_G_ENC_INDEX
>> >            VIDIOC_QBUF/DQBUF/QUERYBUF/PREPARE_BUFS
>> >            VIDIOC_STREAMON/OFF
>> >
>> > So basically cropping, compression encoder/decoder control and actual
>> > streaming. And the subdev and media API is also not tested, although
>> > those might be beyond the scope of this utility anyway.
>> >
>> > Everything else is now tested fairly exhaustively.
>> >
>> >> 2) ... and some larger discussion items have been raised, Eg.
>> >> Absorbing more of the V4L2 kernel infrastructure into the vc8x0 driver
>> >> vs a fairly self-contained driver with very limited opportunities for
>> >> future breakage.
>> >>
>> >> Are you really willing to say that all drivers, with unique and new
>> >> pieces of silicon, need to be split out into independent modules,
>> >> adopting a subdev type interfaces or mainline merge is refused? It's
>> >> not that I'm asking you to merge duplicate functionality, duplicate
>> >> driver code, replicating functionality for new hardware or for an
>> >> existing modules (tuner/demod/whatever). (Like has already happened in
>> >> the past - 18271 for example).
>> >
>> > Speaking for myself, I would probably NACK it, yes. I would hate to do it,
>> > but there are IMHO good technical reasons why the ad7441 code should be
>> > implemented as a subdev driver.
>>
>> I hear you. In the spirit of co-operation I'll take a shot at turning
>> ad7441 into a subdev and see what odd problems shake out of the
>> process. I should be clear that the resulting subdevice will likely be
>> very 820 specific in terms of configuration, but it's a reasonable
>> cut-point.
>
> That's a perfectly valid approach. Frankly, I think it is counter productive
> to try and make a more general implementation. You only end up with code that
> you can't test on your hardware. Such code generally suffers badly from bitrot
> over time.
>
>> >
>> >> If the answer is Yes, then my second questions is:
>> >>
>> >> Assuming the comments / issues mentioned in #1 are addressed, are you
>> >> really willing to stand up in front of the world-wide Kernel
>> >> development community and justify why a driver that passes user-facing
>> >> v4l2-compliance tests, is fairly clean, passes 99% of the reasonable
>> >> checkpatch rules, is fully operational, cannot be merged? Really? Is
>> >> this truly an illegal or inappropriate driver implementation that
>> >> would prohibit mainline merge?
>> >
>> > Yes. Currently nobody else uses the ad7441 but the viewcast driver. So
>> > splitting it up really shouldn't be too much of a problem: you don't have
>> > to take care of anyone else, and it only has to support the functionality
>> > that you need right now. And as long as nobody else uses that driver it
>> > shouldn't make a difference to you maintenance-wise.
>>
>> Fair enough.
>>
>> >
>> > But *if* someone else comes along then that will help them enormously if
>> > an ad7441 driver already exists. We definitely do not want to have duplicate
>> > drivers in the kernel for i2c devices, so either they or you would have to
>> > split up the ad7441 driver from the ViewCast driver, and what are the chances
>> > of that ever happening? Slim to none.
>> >
>> > You just want to get your driver merged, which is perfectly understandable,
>> > but I also want to ensure that whatever gets merged can also be reused by
>> > others, where applicable.
>> >
>> > In addition to that I have to say that I have been working with Analog Devices
>> > i2c receivers and transmitters for the past 4-5 years, and these things are
>> > complex. I consider it very unlikely that your ad7441 driver covers the full
>> > functionality of the ad7441. By implementing it as a separate driver it will
>>
>> Yes.
>>
>> > be much easier for others to work on it and improve it. Yes, that might
>> > require you to do the occasional testing, but hopefully that will improve the
>> > functionality of the ViewCast driver as well by e.g. supporting more formats,
>> > have better colorspace handling, or whatever.
>>
>> <snip>
>>
>> > Also note that the Analog Devices receivers/transmitters are fairly popular,
>> > particularly within the embedded hardware world. So it wouldn't surprise me
>> > at all if other products will appear that want to use it.
>>
>> (7441 commentary removed)
>>
>> Agreed.
>>
>> >
>> > BTW, we are talking about the adv7441a, right?
>> > See here: http://ez.analog.com/docs/DOC-1546
>>
>> Yes.
>>
>> >
>> > There is also a chip called ad7441, but that seems to be something else.
>> > AD has the annoying habit of renaming chips, but at least they've started
>> > making their datasheets freely available, which is very good news for linux.
>>
>> Force of habit on my part, it's the adv7441a.
>
> It would be nice if you can do a search and replace so that the correct name
> is used in the sources.

Yeah, I'll take of that.

>
> And a comment in the source with the URL of the datasheets as I gave above
> is very useful as well. Analog Devices did/are doing a good job when it comes
> to publishing datasheets.
>
>> >
>> >> The ViewCast 820 is a (circa) $1800 video capture card. It's not the
>> >> kind of hardware that everyone has laying around for regression
>> >> testing purposes. If I 1) split this up and people begin to absorb
>> >> ad7441 functionality into other designs, and start patching it and 2)
>> >> adopt the subdev framework for that matter... then nobody is able to
>> >> regression test the impact to the 820. That puts an incredible amount
>> >> of burden on me. I'm attempting to mitigate all of this risk, but also
>> >> provide a GPL driver so everyone can benefit - without creating a
>> >> future maintenance / regression problem, by relying on subsystems the
>> >> driver simply doesn't need.
>> >
>> > What you are basically saying is that you don't want to split it up because
>> > if you do, then other people might reuse the code, change it, and might cause
>> > you a lot of work.
>>
>> Yes.
>>
>> >
>> > What I am saying is that if you split it up, then other people might reuse it,
>> > improve it and with a relatively small amount of work improve the ViewCast
>> > 820 support as well.
>>
>> ... and it would require regression testing on every change.
>>
>> >
>> > I suspect your view of the amount of work it might cost you to test changes
>> > from other people is too pessimistic. It's based on your experiences with the
>> > cx25840, but from my perspective the cx25840 is the exception, not the rule.
>>
>> My comments are largely influenced by the 25840, and we both agree
>> we've stretched that driver too far and too thinly, beyond reasonable
>> use - to the point where we're causing regressions even with small
>> amounts of rework. In hindsight we all know not to let that happen
>> again - but these things have a habit of slowly creeping up on you and
>> they're a problem before you know it.
>>
>> My goal is/was to head that off right at the outset, but without
>> limiting anyone else by my actions. I've implemented just enough of
>> the 7441 for what the 820 needs. A full blown subdev implementation
>> (for all features) is beyond the 820 needs, out of scope. I'll take a
>> shot at converting the limited functionality into a subdev as-is,
>> let's see. If you accept that it's likely to be fairly 820 specific
>> (not unreasonable to begin with) then that's a reasonable compromise.
>
> As I mentioned above, that's a perfectly reasonable approach, and actually
> one that I would recommend regardless.
>
> I have a few remarks with regards to converting the 7441 driver to a subdev
> driver: I recommend doing this last and fixing all other items first. The
> reason is that you probably need two additional pieces of code. The first
> is some additional API support to handle HDMI/DVI/etc. connectors correctly,
> esp. with respect to hotplug support and EDID support. I'll post what is
> hopefully the final pull request for this infrastructure tomorrow. See in the
> meantime my hdmi3 tree:

Noted.

In the meantime, here's the compliance report for the current driver:

http://git.kernellabs.com/?p=stoth/media_tree.git;a=shortlog;h=refs/heads/o820e

I have a couple of questions...

-bash-4.1$ ./v4l2-compliance -d /dev/video0
Driver Info:
	Driver name   : vc8x0
	Card type     : ViewCast 820e
	Bus info      : PCIe:0000:02:00.0
	Driver version: 3.0.1
	Capabilities  : 0x84020001
		Video Capture
		Audio
		Streaming
		Device Capabilities
	Device Caps   : 0x04020001
		Video Capture
		Audio
		Streaming

Compliance test for device /dev/video0 (not using libv4l2):

Required ioctls:
	test VIDIOC_QUERYCAP: OK

Allow for multiple opens:
	test second video open: OK
	test VIDIOC_QUERYCAP: OK
		fail: v4l2-compliance.cpp(323): doioctl(node, VIDIOC_G_PRIORITY, &prio)
	test VIDIOC_G/S_PRIORITY: FAIL

^^^ If I read the compliance test correctly then I think this may be a
bug in the tool. The driver doesn't support g_priority so why mark a
failure?


Debug ioctls:
	test VIDIOC_DBG_G_CHIP_IDENT: Not Supported
	test VIDIOC_DBG_G/S_REGISTER: Not Supported
	test VIDIOC_LOG_STATUS: Not Supported


Debuggung was removed as part of removing the /proc support, but
likely this will return in the form of the above
calls when the next major rev of this 820 card ships (and the driver
is subsequently asked to support a variation of the 820 hardware).

Input ioctls:
	test VIDIOC_G/S_TUNER: Not Supported
	test VIDIOC_G/S_FREQUENCY: Not Supported
	test VIDIOC_S_HW_FREQ_SEEK: OK
	test VIDIOC_ENUMAUDIO: OK
	test VIDIOC_G/S/ENUMINPUT: OK
	test VIDIOC_G/S_AUDIO: OK
	Inputs: 7 Audio Inputs: 1 Tuners: 0

Output ioctls:
	test VIDIOC_G/S_MODULATOR: Not Supported
	test VIDIOC_G/S_FREQUENCY: Not Supported
	test VIDIOC_ENUMAUDOUT: Not Supported
	test VIDIOC_G/S/ENUMOUTPUT: Not Supported
	test VIDIOC_G/S_AUDOUT: Not Supported
	Outputs: 0 Audio Outputs: 0 Modulators: 0

Control ioctls:
		fail: v4l2-test-controls.cpp(275): does not support V4L2_CTRL_FLAG_NEXT_CTRL
	test VIDIOC_QUERYCTRL/MENU: FAIL
	test VIDIOC_G/S_CTRL: OK
	test VIDIOC_G/S/TRY_EXT_CTRLS: Not Supported
	test VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: Not Supported
	test VIDIOC_G/S_JPEGCOMP: Not Supported
	Standard Controls: 0 Private Controls: 0

^^^ I'm not sure how to handle V4L2_CTRL_FLAG_NEXT_CTRL. I've read the
spec a couple of times
and I think I just don't get it. I don't have any ext ctrls and I
think I'm returning EINVAL at the right moment.
If it's not mandatory then why the FAIL?

Input/Output configuration ioctls:
		fail: v4l2-test-io-config.cpp(63): could set standard to ATSC, which
is not supported anymore
		fail: v4l2-test-io-config.cpp(126): STD failed for input 0.
	test VIDIOC_ENUM/G/S/QUERY_STD: FAIL
	test VIDIOC_ENUM/G/S/QUERY_DV_PRESETS: Not Supported
	test VIDIOC_ENUM/G/S/QUERY_DV_TIMINGS: Not Supported
	test VIDIOC_DV_TIMINGS_CAP: Not Supported


^^^ See my comments on gstreamer failing to operate if tvnorms is
non-zero. I have a feeling something in
the core objects to this. I tried NTSC, I also have g_std returning
NTSC, but gstreamer refuses to negotiate
a format/std unless 0 is specified..... And so the test fails. my
s_std is passed 0 for the ID so while the code
exists to EINVAL an attempt on setting ATSC, it's not triggering.

Format ioctls:
		fail: v4l2-test-formats.cpp(252): duplicate format 56595559
	test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: FAIL

^^^ This is don't get. The compliance code implies you should never
have a duplicate pixel format. Perhaps because I have multiple
identical width/height formats with differing framerates and this
confuses things.

	test VIDIOC_G/S_PARM: OK
	test VIDIOC_G_FBUF: Not Supported
	test VIDIOC_G_FMT: OK
		fail: v4l2-test-formats.cpp(549): VBI Capture is valid, but no
TRY_FMT was implemented
	test VIDIOC_TRY_FMT: FAIL
		fail: v4l2-test-formats.cpp(600): Video Capture is valid, but no
S_FMT was implemented
	test VIDIOC_S_FMT: FAIL
	test VIDIOC_G_SLICED_VBI_CAP: Not Supported

^^^ I've looked at the S_FMT many many times and tried a few
workaround. effectively the compliance tool sets the struct to FF,
passed ANY_FIELD and sets set. I refuse the call in the driver, which
I think it's bogus. The Spec is unclear what to do but the compliance
tool still fails. If you have thoughts on this then I'd appreciate it.

Buffer ioctls:
		fail: v4l2-test-buffers.cpp(76): can_stream && !mmap_valid && !userptr_valid
	test VIDIOC_REQBUFS/CREATE_BUFS: FAIL
	test read/write:
	test read/write: OK

The tool shows: fail_on_test(can_stream && !mmap_valid && !userptr_valid);

^^^^ I'm drawing a blank on this failure. I don't deal with either of
these, they're deep in video buf. A bogus error, or something I've
overlooked in videobuf1?

Total: 36, Succeeded: 29, Failed: 7, Warnings: 0

(VBI compliance has a couple more failures I'll take care of this afternoon).

Regards,

- Steve
  

On Thu August 16 2012 19:39:51 Steven Toth wrote:
> >> So, I've ran v4l2-compliance and it pointed out a few things that I've
> >> fixed, but it also does a few things that (for some reason) I can't
> >> seem to catch. One particular test is on (iirc) s_fmt. It attempts to
> >> set ATSC but by ioctl callback never receives ATSC in the norm/id arg,
> >> it actually receives 0x0. This feels more like a bug in the test.
> >> Either way, I have some if (std & ATSC) return -EINVAL, but it still
> >> appears to fail the test.
> 
> Oddly enough. If I set tvnorms to something valid, then compliance
> passes but gstreamer
> fails to run, looks like some kind of confusion about either the
> current established
> norm, or a failure to establish a norm.
> 
> For the time being I've set tvnorms to 0 (with a comment) and removed
> current_norm.

Well, this needs to be sorted, because something is clearly amiss.

> >
> > I think it might be because vdev->tvnorms isn't set for the video node.
> > It's set for the VBI node, though. If tvnorms is 0, then ENUMSTD will
> > probably return an empty list, and that might be the cause of the ATSC
> > test. I also see that current_norm is used: don't do that. Instead store
> > the current standard yourself and return it in g_std. I'm slowly phasing
> > out current_norm because 1) it's ugly and 2) it doesn't work if you have
> > both a vbi and a video node.
> 
> Done.
> 
> >
> >> I see some tests which report failure (testing videobuf) but given
> >> that I essentially pass the ioctl directly into the videbug core, very
> >> much like every oher driver I've ever done, it's probably either a
> >> quirk of the tool, or something inside videobuf core itself that needs
> >> some adjustment. (userptr/mmap for capture or output buffers related
> >> test).
> >
> > videobuf does not follow the spec in several areas (most notably and extremely
> > annoyingly it does not support calling REQBUFS with a count of 0). So any
> > driver that uses videobuf will fail a number of tests in v4l2-compliance.
> 
> Compliance as of today (with changes), now gives:
> 
> "Buffer ioctls:
> 		fail: v4l2-test-buffers.cpp(76): can_stream && !mmap_valid && !userptr_valid
> 	test VIDIOC_REQBUFS/CREATE_BUFS: FAIL
> 	test read/write: OK"

Yeah, that's because videobuf is used instead of vb2.

<snip>

> In the meantime, here's the compliance report for the current driver:
> 
> http://git.kernellabs.com/?p=stoth/media_tree.git;a=shortlog;h=refs/heads/o820e
> 
> I have a couple of questions...
> 
> -bash-4.1$ ./v4l2-compliance -d /dev/video0
> Driver Info:
> 	Driver name   : vc8x0
> 	Card type     : ViewCast 820e
> 	Bus info      : PCIe:0000:02:00.0
> 	Driver version: 3.0.1
> 	Capabilities  : 0x84020001
> 		Video Capture
> 		Audio
> 		Streaming
> 		Device Capabilities
> 	Device Caps   : 0x04020001
> 		Video Capture
> 		Audio
> 		Streaming
> 
> Compliance test for device /dev/video0 (not using libv4l2):
> 
> Required ioctls:
> 	test VIDIOC_QUERYCAP: OK
> 
> Allow for multiple opens:
> 	test second video open: OK
> 	test VIDIOC_QUERYCAP: OK
> 		fail: v4l2-compliance.cpp(323): doioctl(node, VIDIOC_G_PRIORITY, &prio)
> 	test VIDIOC_G/S_PRIORITY: FAIL
> 
> ^^^ If I read the compliance test correctly then I think this may be a
> bug in the tool. The driver doesn't support g_priority so why mark a
> failure?

The tool is more strict in some places than the spec is. One of those cases
is that is requires that priority handling is implemented. The reason for that
is 1) if you use the latest framework support (struct v4l2_fh in particular),
then you get priority handling for free by just setting a single bit, so there
is no excuse not to implement it, and 2) the reason few if any applications use
it is that too few drivers implemented it, so apps couldn't rely on it. It's
again a chicken and egg problem and the only way to improve matters is to have
a check that clearly tells you to add support for this.

> 
> 
> Debug ioctls:
> 	test VIDIOC_DBG_G_CHIP_IDENT: Not Supported
> 	test VIDIOC_DBG_G/S_REGISTER: Not Supported
> 	test VIDIOC_LOG_STATUS: Not Supported
> 
> 
> Debuggung was removed as part of removing the /proc support, but
> likely this will return in the form of the above
> calls when the next major rev of this 820 card ships (and the driver
> is subsequently asked to support a variation of the 820 hardware).

OK.

> Input ioctls:
> 	test VIDIOC_G/S_TUNER: Not Supported
> 	test VIDIOC_G/S_FREQUENCY: Not Supported
> 	test VIDIOC_S_HW_FREQ_SEEK: OK
> 	test VIDIOC_ENUMAUDIO: OK
> 	test VIDIOC_G/S/ENUMINPUT: OK
> 	test VIDIOC_G/S_AUDIO: OK
> 	Inputs: 7 Audio Inputs: 1 Tuners: 0
> 
> Output ioctls:
> 	test VIDIOC_G/S_MODULATOR: Not Supported
> 	test VIDIOC_G/S_FREQUENCY: Not Supported
> 	test VIDIOC_ENUMAUDOUT: Not Supported
> 	test VIDIOC_G/S/ENUMOUTPUT: Not Supported
> 	test VIDIOC_G/S_AUDOUT: Not Supported
> 	Outputs: 0 Audio Outputs: 0 Modulators: 0
> 
> Control ioctls:
> 		fail: v4l2-test-controls.cpp(275): does not support V4L2_CTRL_FLAG_NEXT_CTRL
> 	test VIDIOC_QUERYCTRL/MENU: FAIL
> 	test VIDIOC_G/S_CTRL: OK
> 	test VIDIOC_G/S/TRY_EXT_CTRLS: Not Supported
> 	test VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: Not Supported
> 	test VIDIOC_G/S_JPEGCOMP: Not Supported
> 	Standard Controls: 0 Private Controls: 0
> 
> ^^^ I'm not sure how to handle V4L2_CTRL_FLAG_NEXT_CTRL. I've read the
> spec a couple of times
> and I think I just don't get it. I don't have any ext ctrls and I
> think I'm returning EINVAL at the right moment.
> If it's not mandatory then why the FAIL?

This is another place where the tool is more strict than the spec: by requiring
that extended controls are also implemented it ensures a more consistent API
towards applications. By using the control framework you get all this for free,
so it is also a nice way of enforcing the use of the proper frameworks.

And you should use the control framework anyway: it's trivial for this driver
to implement.

By using struct v4l2_fh and the control framework you will get automatic support
for priority handling, control events and extended controls without having to
think about it.

> 
> Input/Output configuration ioctls:
> 		fail: v4l2-test-io-config.cpp(63): could set standard to ATSC, which
> is not supported anymore
> 		fail: v4l2-test-io-config.cpp(126): STD failed for input 0.
> 	test VIDIOC_ENUM/G/S/QUERY_STD: FAIL
> 	test VIDIOC_ENUM/G/S/QUERY_DV_PRESETS: Not Supported
> 	test VIDIOC_ENUM/G/S/QUERY_DV_TIMINGS: Not Supported
> 	test VIDIOC_DV_TIMINGS_CAP: Not Supported
> 
> 
> ^^^ See my comments on gstreamer failing to operate if tvnorms is
> non-zero. I have a feeling something in
> the core objects to this. I tried NTSC, I also have g_std returning
> NTSC, but gstreamer refuses to negotiate
> a format/std unless 0 is specified..... And so the test fails. my
> s_std is passed 0 for the ID so while the code
> exists to EINVAL an attempt on setting ATSC, it's not triggering.

I would have to see the latest code, I guess.

> 
> Format ioctls:
> 		fail: v4l2-test-formats.cpp(252): duplicate format 56595559
> 	test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: FAIL
> 
> ^^^ This is don't get. The compliance code implies you should never
> have a duplicate pixel format. Perhaps because I have multiple
> identical width/height formats with differing framerates and this
> confuses things.

ENUM_FMT returns duplicate formats, and it shouldn't do that.

With ENUM_FRAMESIZES you can enumerate the available framesizes for each
format, and with ENUM_FRAMEINTERVALS you enumerate the available framerates
for each format/size combination.

I know the spec doesn't say anything about whether enum_fmt can return duplicate
formats, but it makes no sense IMHO. Enumeration implies that you enumerate over
unique values.

> 
> 	test VIDIOC_G/S_PARM: OK
> 	test VIDIOC_G_FBUF: Not Supported
> 	test VIDIOC_G_FMT: OK
> 		fail: v4l2-test-formats.cpp(549): VBI Capture is valid, but no
> TRY_FMT was implemented
> 	test VIDIOC_TRY_FMT: FAIL
> 		fail: v4l2-test-formats.cpp(600): Video Capture is valid, but no
> S_FMT was implemented
> 	test VIDIOC_S_FMT: FAIL
> 	test VIDIOC_G_SLICED_VBI_CAP: Not Supported
> 
> ^^^ I've looked at the S_FMT many many times and tried a few
> workaround. effectively the compliance tool sets the struct to FF,
> passed ANY_FIELD and sets set. I refuse the call in the driver, which
> I think it's bogus. The Spec is unclear what to do but the compliance
> tool still fails. If you have thoughts on this then I'd appreciate it.

This is an area that will be clarified during the workshop. The question
is: can S_FMT ever return an error if you give it an invalid format. The
spec currently allows it, but it is my believe (and that of others as
well) that that should change. S_FMT (and TRY_FMT) should always return
some valid format and never an error (unless the type is wrong or streaming
is in progress and you can't change the format midstream).

The v4l2-compliance tool is currently assuming that behavior, so it sets
all fields with bogus values (and field to ANY to test whether the driver
can handle that value correctly), and checks whether it can do a S_FMT
without it returning an error.

> 
> Buffer ioctls:
> 		fail: v4l2-test-buffers.cpp(76): can_stream && !mmap_valid && !userptr_valid
> 	test VIDIOC_REQBUFS/CREATE_BUFS: FAIL
> 	test read/write:
> 	test read/write: OK
> 
> The tool shows: fail_on_test(can_stream && !mmap_valid && !userptr_valid);
> 
> ^^^^ I'm drawing a blank on this failure. I don't deal with either of
> these, they're deep in video buf. A bogus error, or something I've
> overlooked in videobuf1?

It's a consequence of using videobuf instead of vb2. This code is testing
REQBUFS with a count of 0, which isn't supported by videobuf. Switch to vb2
and this will work. It's one of the reasons why vb2 was made.

Regards,

	Hans

> 
> Total: 36, Succeeded: 29, Failed: 7, Warnings: 0
> 
> (VBI compliance has a couple more failures I'll take care of this afternoon).
> 
> Regards,
> 
> - Steve
> 
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html