All of lore.kernel.org
 help / color / mirror / Atom feed
From: Devin Heitmueller <dheitmueller@kernellabs.com>
To: Hans Verkuil <hverkuil@xs4all.nl>
Cc: Mauro Carvalho Chehab <mchehab@redhat.com>,
	linux-media@vger.kernel.org,
	Simon Farnsworth <simon.farnsworth@onelan.co.uk>,
	Steven Toth <stoth@kernellabs.com>,
	Andy Walls <awalls@md.metrocast.net>
Subject: Re: [git:v4l-dvb/for_v2.6.40] [media] cx18: mmap() support for raw YUV video capture
Date: Tue, 3 May 2011 08:49:43 -0400	[thread overview]
Message-ID: <BANLkTinjYo0zW56+vEMDciXkdk9gePOZnQ@mail.gmail.com> (raw)
In-Reply-To: <201105022331.29142.hverkuil@xs4all.nl>

On Mon, May 2, 2011 at 5:31 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
> It's also a good idea if the author of a patch pings the list if there
> has been no feedback after one or two weeks. It's easy to forget patches,
> people can be on vacation, be sick, or in the case of Andy, have a family
> emergency.

In principle I agree with you, and I was actually surprised to hear it
was merged.  That said, what's done is done so we need to focus on
where to go from here.

>> Likewise, I know there have indeed been cases in the past where code
>> got upstream that caused regressions (in fact, you have personally
>> been responsible for some of these if I recall).
>>
>> Let's not throw the baby out with the bathwater.  If there are real
>> structural issues with the patch, then let's get them fixed.  But if
>> we're just talking about a few minor "unused variable" type of
>> aesthetic issues, then that shouldn't constitute reverting the commit.
>>  Do your review, and if an additional patch is needed with a half
>> dozen removals of dead/unused code, then so be it.
>
> Well, one structural thing I am not at all happy about (but it is Andy's
> call) is that it uses videobuf instead of vb2. Since this patch only deals
> with YUV it shouldn't be hard to use vb2. The problem with videobuf is that
> it violates the V4L2 spec in several places so I would prefer not to use
> videobuf in cx18. If only because converting cx18 to vb2 later will change
> the behavior of the stream I/O (VIDIOC_REQBUFS in particular), which is
> something I would like to avoid if possible.
>
> I know that Andy started work on vb2 in cx18 for all stream types (not just
> YUV). I have no idea of the current state of that work. But it might be a
> good starting point to use this patch and convert it to vb2. Later Andy can
> add vb2 support for the other stream types.

Sure Hans.  Let me just dig into my collection of 30+ products and
grab one that has already been converted to VB2 which I can use as a
reference for porting.  Should be simple enough...

cx88: nope
cx23885: nope
cx18: nope
ivtv: nope
em28xx: nope
au0828: nope
pvrusb2: nope
cx231xx: nope
saa7134: nope
saa7164: nope
tm6010: nope
dib0700: nope
bttv: nope

Oh wait, you mean that there aren't *any* non-embedded drivers that
currently implement VB2?  Vivi is the *only* example, and it's not
even real hardware so who knows what issues with the architecture we
might run into?

And exactly what real-world applications has VB2 been validated
against?  Any apps that aren't just a test harness or written by an
SOC vendor making it work against their one piece of embedded
hardware?  Any consumer apps?  Mplayer?  VLC?  Kaffeine?  tvtime?
XawTV?  MeTV?  MythTV?

VB2 may be the future of buffering models and it may actually be
better in the long-run, but if you want to see adoption outside of the
SOC space then you need to prove that it works against real hardware
that isn't an SOC, and demonstrate that it doesn't cause regressions
in real-world applications that people are using today.

Let's talk about what's going to happen in the real world:  the first
guy who actually ports one of the above drivers to VB2 is going to run
into bugs.  He's going to have to work with you to shake out those
bugs.  And it wouldn't surprise me if it exposes some bugs in some
existing applications, which are going to have to be fixed too.  In
the end we'll eventually end up in a better situation, but the cost
will be non-trivial and it will be incurred by people who don't really
give a damn about VB2 since it has little end-user visible benefit.

If you had ported any of the above drivers to the VB2 framework and
demonstrated that it works with existing applications without
modifications, then I think everybody here would breathe much easier.
But the current state today is "experimental, not implemented in any
consumer products or validated in any real-world usage outside of
SOC".

Asking us to be the "guinea pig" for this new framework just because
cx18 is the most recent driver to get a videobuf related patch just
isn't appropriate.

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com

  parent reply	other threads:[~2011-05-03 12:49 UTC|newest]

Thread overview: 38+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <E1QGwlS-0006ys-15@www.linuxtv.org>
2011-05-02 19:11 ` [git:v4l-dvb/for_v2.6.40] [media] cx18: mmap() support for raw YUV video capture Hans Verkuil
2011-05-02 19:21   ` Devin Heitmueller
2011-05-02 19:35   ` Mauro Carvalho Chehab
2011-05-02 19:40     ` Devin Heitmueller
2011-05-02 20:02     ` Hans Verkuil
2011-05-02 20:59       ` Devin Heitmueller
2011-05-02 21:31         ` Hans Verkuil
2011-05-03  1:59           ` Mauro Carvalho Chehab
2011-05-03  2:40           ` Andy Walls
2011-05-03  3:28             ` Mauro Carvalho Chehab
2011-05-03  5:15               ` Hans Verkuil
2011-05-03 11:29                 ` Mauro Carvalho Chehab
2011-05-03 13:07             ` Devin Heitmueller
2011-05-03 12:49           ` Devin Heitmueller [this message]
2011-05-03 13:59             ` Hans Verkuil
2011-05-03 14:26               ` Simon Farnsworth
2011-05-03 15:03               ` Mauro Carvalho Chehab
2011-05-03 16:13                 ` Hans Verkuil
2011-05-03  9:03     ` Simon Farnsworth
2011-05-03 10:56       ` Mauro Carvalho Chehab
2011-05-03 11:57         ` [PATCH] cx18: Clean up mmap() support for raw YUV Simon Farnsworth
2011-05-03 16:24           ` Hans Verkuil
2011-05-03 22:51           ` Andy Walls
2011-05-03 23:01             ` Mauro Carvalho Chehab
2011-05-03 23:38               ` Andy Walls
2011-05-04  0:17                 ` Mauro Carvalho Chehab
2011-05-04  9:32             ` Simon Farnsworth
2011-05-04 11:31               ` Mauro Carvalho Chehab
2011-05-04 11:39                 ` [PATCH] cx18: Bump driver version to 1.5.0 Simon Farnsworth
2011-05-04 12:20                   ` Andy Walls
2011-05-05 12:42                 ` [PATCH] cx18: Fix warnings introduced during cleanup Simon Farnsworth
2011-05-05 13:41                   ` Mauro Carvalho Chehab
2011-05-05 13:44                     ` Simon Farnsworth
2011-05-10 13:49                     ` [PATCH] cx18: Move spinlock and vb_type initialisation into stream_init Simon Farnsworth
2011-05-20 23:21                       ` Mauro Carvalho Chehab
2011-05-05 11:41               ` [PATCH] cx18: Clean up mmap() support for raw YUV Mauro Carvalho Chehab
2011-05-05 12:44                 ` Simon Farnsworth
2011-05-05 13:39                   ` Mauro Carvalho Chehab

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=BANLkTinjYo0zW56+vEMDciXkdk9gePOZnQ@mail.gmail.com \
    --to=dheitmueller@kernellabs.com \
    --cc=awalls@md.metrocast.net \
    --cc=hverkuil@xs4all.nl \
    --cc=linux-media@vger.kernel.org \
    --cc=mchehab@redhat.com \
    --cc=simon.farnsworth@onelan.co.uk \
    --cc=stoth@kernellabs.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.