Re: [GIT PULL] ViewCast O820E capture support added

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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.

-- 
Steven Toth - Kernel Labs
http://www.kernellabs.com
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux