[REVIEW] au0828-video.c
Hi Shuah,
This is the video.c review with your patch applied.
/*
* Auvitek AU0828 USB Bridge (Analog video support)
*
* Copyright (C) 2009 Devin Heitmueller dheitmuel...@linuxtv.org
* Copyright (C) 2005-2008 Auvitek International, Ltd.
*
* This program is free software; you can redistribute it and/or
* modify it under the terms of the GNU General Public License
* As published by the Free Software Foundation; either version 2
* of the License, or (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU General Public License for more details.
*
* You should have received a copy of the GNU General Public License
* along with this program; if not, write to the Free Software
* Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
* 02110-1301, USA.
*/
/* Developer Notes:
*
* VBI support is not yet working
I'll see if I can get this to work quickly. If not, then we should
probably just strip the VBI support from this driver. It's pointless to
have non-functioning VBI support.
* The hardware scaler supported is unimplemented
* AC97 audio support is unimplemented (only i2s audio mode)
*
*/
#include au0828.h
#include linux/module.h
#include linux/slab.h
#include linux/init.h
#include linux/device.h
#include media/v4l2-common.h
#include media/v4l2-ioctl.h
#include media/v4l2-event.h
#include media/tuner.h
#include au0828-reg.h
snip
static int queue_setup(struct vb2_queue *vq, const struct v4l2_format *fmt,
unsigned int *nbuffers, unsigned int *nplanes,
unsigned int sizes[], void *alloc_ctxs[])
{
struct au0828_dev *dev = vb2_get_drv_priv(vq);
unsigned long size;
if (fmt)
size = fmt-fmt.pix.sizeimage;
else
size =
(dev-width * dev-height * dev-bytesperline + 7) 3;
This is wrong. It should be 'size = dev-bytesperline * dev-height;'
If fmt is set, then you need to check if the fmt size is large enough for
the current format.
if (size == 0)
This should be 'size img_size'.
return -EINVAL;
if (0 == *nbuffers)
*nbuffers = 32;
Bogus check.
*nplanes = 1;
sizes[0] = size;
return 0;
}
I would rewrite this function as:
{
struct au0828_dev *dev = vb2_get_drv_priv(vq);
unsigned img_size = dev-bytesperline * dev-height;
unsigned long size;
size = fmt ? fmt-fmt.pix.sizeimage : img_size;
if (size img_size)
return -EINVAL;
*nplanes = 1;
sizes[0] = size;
return 0;
}
static int
buffer_prepare(struct vb2_buffer *vb)
{
struct au0828_buffer *buf = container_of(vb, struct au0828_buffer, vb);
struct au0828_dev*dev = vb2_get_drv_priv(vb-vb2_queue);
buf-length = (dev-width * dev-height * 16 + 7) 3;
Use buf-length = dev-bytesperline * dev-height;
if (vb2_plane_size(vb, 0) buf-length) {
pr_err(%s data will not fit into plane (%lu %lu)\n,
__func__, vb2_plane_size(vb, 0), buf-length);
return -EINVAL;
}
vb2_set_plane_payload(buf-vb, 0, buf-length);
return 0;
}
static void
buffer_queue(struct vb2_buffer *vb)
{
struct au0828_buffer*buf = container_of(vb,
struct au0828_buffer,
vb);
struct au0828_dev *dev = vb2_get_drv_priv(vb-vb2_queue);
struct au0828_dmaqueue *vidq= dev-vidq;
unsigned long flags = 0;
buf-mem = vb2_plane_vaddr(vb, 0);
buf-length = vb2_plane_size(vb, 0);
spin_lock_irqsave(dev-slock, flags);
list_add_tail(buf-list, vidq-active);
spin_unlock_irqrestore(dev-slock, flags);
}
static int au0828_i2s_init(struct au0828_dev *dev)
{
/* Enable i2s mode */
au0828_writereg(dev, AU0828_AUDIOCTRL_50C, 0x01);
return 0;
}
/*
* Auvitek au0828 analog stream enable
*/
static int au0828_analog_stream_enable(struct au0828_dev *d)
{
struct usb_interface *iface;
int ret, h, w;
dprintk(1, au0828_analog_stream_enable called\n);
iface = usb_ifnum_to_if(d-usbdev, 0);
if (iface iface-cur_altsetting-desc.bAlternateSetting != 5) {
dprintk(1, Changing intf#0 to alt 5\n);
/* set au0828 interface0 to AS5 here again */
ret = usb_set_interface(d-usbdev, 0, 5);
if (ret 0) {
pr_info(Au0828 can't set alt setting to 5!\n);
return -EBUSY;
}
}
h = d-height / 2 + 2;
w =
Re: [REVIEW] au0828-video.c
Em Fri, 12 Dec 2014 11:16:01 +0100 Hans Verkuil hverk...@xs4all.nl escreveu: Hi Shuah, This is the video.c review with your patch applied. /* * Auvitek AU0828 USB Bridge (Analog video support) * * Copyright (C) 2009 Devin Heitmueller dheitmuel...@linuxtv.org * Copyright (C) 2005-2008 Auvitek International, Ltd. * * This program is free software; you can redistribute it and/or * modify it under the terms of the GNU General Public License * As published by the Free Software Foundation; either version 2 * of the License, or (at your option) any later version. * * This program is distributed in the hope that it will be useful, * but WITHOUT ANY WARRANTY; without even the implied warranty of * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the * GNU General Public License for more details. * * You should have received a copy of the GNU General Public License * along with this program; if not, write to the Free Software * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA * 02110-1301, USA. */ /* Developer Notes: * * VBI support is not yet working I'll see if I can get this to work quickly. If not, then we should probably just strip the VBI support from this driver. It's pointless to have non-functioning VBI support. This is a left-over. VBI support works on this driver. I tested. Probably, the patches that added VBI support forgot to remove the above notice. /* This function ensures that video frames continue to be delivered even if the ITU-656 input isn't receiving any data (thereby preventing applications such as tvtime from hanging) */ Why would tvtime be hanging? Make a separate patch that just removes all this timeout nonsense. If there are no frames, then tvtime (and any other app) should just wait for frames to arrive. And ctrl-C should always be able to break the app (or they can timeout themselves). It's not the driver's responsibility to do this and it only makes the code overly complex. Well, we should not cause regressions on userspace. If removing this check will cause tvtime to hang, we should keep it. Btw, the same kind of test used to be at vivi and other drivers. I think we removed it there some time ago, so maybe either it was a VB1 bug or this got fixed at tvtime. Regards, Mauro -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [REVIEW] au0828-video.c
On 12/12/2014 01:49 PM, Mauro Carvalho Chehab wrote: Em Fri, 12 Dec 2014 11:16:01 +0100 Hans Verkuil hverk...@xs4all.nl escreveu: Hi Shuah, This is the video.c review with your patch applied. /* * Auvitek AU0828 USB Bridge (Analog video support) * * Copyright (C) 2009 Devin Heitmueller dheitmuel...@linuxtv.org * Copyright (C) 2005-2008 Auvitek International, Ltd. * * This program is free software; you can redistribute it and/or * modify it under the terms of the GNU General Public License * As published by the Free Software Foundation; either version 2 * of the License, or (at your option) any later version. * * This program is distributed in the hope that it will be useful, * but WITHOUT ANY WARRANTY; without even the implied warranty of * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the * GNU General Public License for more details. * * You should have received a copy of the GNU General Public License * along with this program; if not, write to the Free Software * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA * 02110-1301, USA. */ /* Developer Notes: * * VBI support is not yet working I'll see if I can get this to work quickly. If not, then we should probably just strip the VBI support from this driver. It's pointless to have non-functioning VBI support. This is a left-over. VBI support works on this driver. I tested. Oh wait, now I get it. You are only capturing line 21, not the whole vbi area. That's why vbi_height = 1. Never mind then. Although that comment should indeed be removed. Probably, the patches that added VBI support forgot to remove the above notice. /* This function ensures that video frames continue to be delivered even if the ITU-656 input isn't receiving any data (thereby preventing applications such as tvtime from hanging) */ Why would tvtime be hanging? Make a separate patch that just removes all this timeout nonsense. If there are no frames, then tvtime (and any other app) should just wait for frames to arrive. And ctrl-C should always be able to break the app (or they can timeout themselves). It's not the driver's responsibility to do this and it only makes the code overly complex. Well, we should not cause regressions on userspace. If removing this check will cause tvtime to hang, we should keep it. Obviously if it hangs (i.e. tvtime can't be killed anymore) it is a bug in the driver. But the driver shouldn't start generating bogus frames just because no new frames are arriving, that's just nuts. Btw, the same kind of test used to be at vivi and other drivers. I think we removed it there some time ago, so maybe either it was a VB1 bug or this got fixed at tvtime. Most likely. Regards, Hans -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [REVIEW] au0828-video.c
Em Fri, 12 Dec 2014 13:55:14 +0100 Hans Verkuil hverk...@xs4all.nl escreveu: On 12/12/2014 01:49 PM, Mauro Carvalho Chehab wrote: Em Fri, 12 Dec 2014 11:16:01 +0100 Hans Verkuil hverk...@xs4all.nl escreveu: Hi Shuah, This is the video.c review with your patch applied. /* * Auvitek AU0828 USB Bridge (Analog video support) * * Copyright (C) 2009 Devin Heitmueller dheitmuel...@linuxtv.org * Copyright (C) 2005-2008 Auvitek International, Ltd. * * This program is free software; you can redistribute it and/or * modify it under the terms of the GNU General Public License * As published by the Free Software Foundation; either version 2 * of the License, or (at your option) any later version. * * This program is distributed in the hope that it will be useful, * but WITHOUT ANY WARRANTY; without even the implied warranty of * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the * GNU General Public License for more details. * * You should have received a copy of the GNU General Public License * along with this program; if not, write to the Free Software * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA * 02110-1301, USA. */ /* Developer Notes: * * VBI support is not yet working I'll see if I can get this to work quickly. If not, then we should probably just strip the VBI support from this driver. It's pointless to have non-functioning VBI support. This is a left-over. VBI support works on this driver. I tested. Oh wait, now I get it. You are only capturing line 21, not the whole vbi area. That's why vbi_height = 1. Never mind then. Although that comment should indeed be removed. Probably, the patches that added VBI support forgot to remove the above notice. /* This function ensures that video frames continue to be delivered even if the ITU-656 input isn't receiving any data (thereby preventing applications such as tvtime from hanging) */ Why would tvtime be hanging? Make a separate patch that just removes all this timeout nonsense. If there are no frames, then tvtime (and any other app) should just wait for frames to arrive. And ctrl-C should always be able to break the app (or they can timeout themselves). It's not the driver's responsibility to do this and it only makes the code overly complex. Well, we should not cause regressions on userspace. If removing this check will cause tvtime to hang, we should keep it. Obviously if it hangs (i.e. tvtime can't be killed anymore) it is a bug in the driver. But the driver shouldn't start generating bogus frames just because no new frames are arriving, that's just nuts. If I remember the bug well, what used to happen is that tvtime would wait for a certain amount of time for a frame. If nothing arrives, it stops capturing. The net effect is that tvtime shows no picture. This used to be so bad that tvtime didn't work with vivi at all. The bug used also to manifest there if lots of frames got dropped when, for example, changing from one channel to another. Btw, on a quick look, I'm not seeing any patch at tvtime since we took it over that would be fixing it. So, it was either a VB bug or the bug is still there. Btw, the same kind of test used to be at vivi and other drivers. I think we removed it there some time ago, so maybe either it was a VB1 bug or this got fixed at tvtime. Most likely. Regards, Hans -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [REVIEW] au0828-video.c
Hi Hans,
Thanks for a quick review.
On 12/12/2014 03:16 AM, Hans Verkuil wrote:
Hi Shuah,
This is the video.c review with your patch applied.
/*
* Auvitek AU0828 USB Bridge (Analog video support)
*
* Copyright (C) 2009 Devin Heitmueller dheitmuel...@linuxtv.org
* Copyright (C) 2005-2008 Auvitek International, Ltd.
*
* This program is free software; you can redistribute it and/or
* modify it under the terms of the GNU General Public License
* As published by the Free Software Foundation; either version 2
* of the License, or (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU General Public License for more details.
*
* You should have received a copy of the GNU General Public License
* along with this program; if not, write to the Free Software
* Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
* 02110-1301, USA.
*/
/* Developer Notes:
*
* VBI support is not yet working
I'll see if I can get this to work quickly. If not, then we should
probably just strip the VBI support from this driver. It's pointless to
have non-functioning VBI support.
* The hardware scaler supported is unimplemented
* AC97 audio support is unimplemented (only i2s audio mode)
*
*/
#include au0828.h
#include linux/module.h
#include linux/slab.h
#include linux/init.h
#include linux/device.h
#include media/v4l2-common.h
#include media/v4l2-ioctl.h
#include media/v4l2-event.h
#include media/tuner.h
#include au0828-reg.h
snip
static int queue_setup(struct vb2_queue *vq, const struct v4l2_format *fmt,
unsigned int *nbuffers, unsigned int *nplanes,
unsigned int sizes[], void *alloc_ctxs[])
{
struct au0828_dev *dev = vb2_get_drv_priv(vq);
unsigned long size;
if (fmt)
size = fmt-fmt.pix.sizeimage;
else
size =
(dev-width * dev-height * dev-bytesperline + 7) 3;
This is wrong. It should be 'size = dev-bytesperline * dev-height;'
Will fix it.
If fmt is set, then you need to check if the fmt size is large enough for
the current format.
if (size == 0)
This should be 'size img_size'.
return -EINVAL;
if (0 == *nbuffers)
*nbuffers = 32;
Bogus check.
Will fix it.
*nplanes = 1;
sizes[0] = size;
return 0;
}
I would rewrite this function as:
{
struct au0828_dev *dev = vb2_get_drv_priv(vq);
unsigned img_size = dev-bytesperline * dev-height;
unsigned long size;
size = fmt ? fmt-fmt.pix.sizeimage : img_size;
if (size img_size)
return -EINVAL;
*nplanes = 1;
sizes[0] = size;
return 0;
}
That simplifies it a lot.
static int
buffer_prepare(struct vb2_buffer *vb)
{
struct au0828_buffer *buf = container_of(vb, struct au0828_buffer, vb);
struct au0828_dev*dev = vb2_get_drv_priv(vb-vb2_queue);
buf-length = (dev-width * dev-height * 16 + 7) 3;
Use buf-length = dev-bytesperline * dev-height;
Yes. Will do.
if (vb2_plane_size(vb, 0) buf-length) {
pr_err(%s data will not fit into plane (%lu %lu)\n,
__func__, vb2_plane_size(vb, 0), buf-length);
return -EINVAL;
}
vb2_set_plane_payload(buf-vb, 0, buf-length);
return 0;
}
static void
buffer_queue(struct vb2_buffer *vb)
{
struct au0828_buffer*buf = container_of(vb,
struct au0828_buffer,
vb);
struct au0828_dev *dev = vb2_get_drv_priv(vb-vb2_queue);
struct au0828_dmaqueue *vidq= dev-vidq;
unsigned long flags = 0;
buf-mem = vb2_plane_vaddr(vb, 0);
buf-length = vb2_plane_size(vb, 0);
spin_lock_irqsave(dev-slock, flags);
list_add_tail(buf-list, vidq-active);
spin_unlock_irqrestore(dev-slock, flags);
}
static int au0828_i2s_init(struct au0828_dev *dev)
{
/* Enable i2s mode */
au0828_writereg(dev, AU0828_AUDIOCTRL_50C, 0x01);
return 0;
}
/*
* Auvitek au0828 analog stream enable
*/
static int au0828_analog_stream_enable(struct au0828_dev *d)
{
struct usb_interface *iface;
int ret, h, w;
dprintk(1, au0828_analog_stream_enable called\n);
iface = usb_ifnum_to_if(d-usbdev, 0);
if (iface iface-cur_altsetting-desc.bAlternateSetting != 5) {
dprintk(1, Changing intf#0 to alt 5\n);
/* set au0828 interface0 to AS5 here again */
ret = usb_set_interface(d-usbdev, 0, 5);
if (ret 0) {
pr_info(Au0828 can't set
Re: [REVIEW] au0828-video.c
On 12/12/2014 08:28 AM, Hans Verkuil wrote: On 12/12/2014 04:26 PM, Shuah Khan wrote: On 12/12/2014 06:14 AM, Mauro Carvalho Chehab wrote: Em Fri, 12 Dec 2014 13:55:14 +0100 Hans Verkuil hverk...@xs4all.nl escreveu: On 12/12/2014 01:49 PM, Mauro Carvalho Chehab wrote: Em Fri, 12 Dec 2014 11:16:01 +0100 Hans Verkuil hverk...@xs4all.nl escreveu: Hi Shuah, This is the video.c review with your patch applied. /* * Auvitek AU0828 USB Bridge (Analog video support) * * Copyright (C) 2009 Devin Heitmueller dheitmuel...@linuxtv.org * Copyright (C) 2005-2008 Auvitek International, Ltd. * * This program is free software; you can redistribute it and/or * modify it under the terms of the GNU General Public License * As published by the Free Software Foundation; either version 2 * of the License, or (at your option) any later version. * * This program is distributed in the hope that it will be useful, * but WITHOUT ANY WARRANTY; without even the implied warranty of * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the * GNU General Public License for more details. * * You should have received a copy of the GNU General Public License * along with this program; if not, write to the Free Software * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA * 02110-1301, USA. */ /* Developer Notes: * * VBI support is not yet working I'll see if I can get this to work quickly. If not, then we should probably just strip the VBI support from this driver. It's pointless to have non-functioning VBI support. This is a left-over. VBI support works on this driver. I tested. Oh wait, now I get it. You are only capturing line 21, not the whole vbi area. That's why vbi_height = 1. Never mind then. Although that comment should indeed be removed. Want me to remove the comment with this work or as a separate patch?? Separate, I think. Probably, the patches that added VBI support forgot to remove the above notice. /* This function ensures that video frames continue to be delivered even if the ITU-656 input isn't receiving any data (thereby preventing applications such as tvtime from hanging) */ Why would tvtime be hanging? Make a separate patch that just removes all this timeout nonsense. If there are no frames, then tvtime (and any other app) should just wait for frames to arrive. And ctrl-C should always be able to break the app (or they can timeout themselves). It's not the driver's responsibility to do this and it only makes the code overly complex. Well, we should not cause regressions on userspace. If removing this check will cause tvtime to hang, we should keep it. Obviously if it hangs (i.e. tvtime can't be killed anymore) it is a bug in the driver. But the driver shouldn't start generating bogus frames just because no new frames are arriving, that's just nuts. If I remember the bug well, what used to happen is that tvtime would wait for a certain amount of time for a frame. If nothing arrives, it stops capturing. The net effect is that tvtime shows no picture. This used to be so bad that tvtime didn't work with vivi at all. The bug used also to manifest there if lots of frames got dropped when, for example, changing from one channel to another. Btw, on a quick look, I'm not seeing any patch at tvtime since we took it over that would be fixing it. So, it was either a VB bug or the bug is still there. Btw, the same kind of test used to be at vivi and other drivers. I think we removed it there some time ago, so maybe either it was a VB1 bug or this got fixed at tvtime. I take it that we decided to keep the timeout handling for now. No, tvtime no longer hangs if no frames arrive, so there is no need for this timeout handling. I'd strip it out, which can be done in a separate patch. Will do. This will reduce the complexity other drives don't have. -- Shuah -- Shuah Khan Sr. Linux Kernel Developer Samsung Open Source Group Samsung Research America (Silicon Valley) shua...@osg.samsung.com | (970) 217-8978 -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [REVIEW] au0828-video.c
No, tvtime no longer hangs if no frames arrive, so there is no need for this timeout handling. I'd strip it out, which can be done in a separate patch. Did you actually try it? Do you have some patches to tvtime which aren't upstream? I wrote the comment in question (and added the associated code). The issue is that tvtime does *everything* in a single thread (except the recent ALSA audio work), that includes servicing the video/vbi devices as well as the user interface. That thread blocks on a DQBUF ioctl until data arrives, and thus if frames are not being delivered it will hang the entire tvtime user interface. Now you can certainly argue that is a bad design decision, but it's been that way for 15+ years, so we can't break it now. Hence why I generate dummy frames on a timeout if the decoder isn't delivering video. Unfortunately the au8522 doesn't have a free running mode (i.e. blue screen if no video), which is why most of the other devices work fine (decoders by Conexant, NXP, Trident, etc all have such functionality). Don't get me wrong - I *hate* that I had to put that timer crap in the driver, but it was necessary to be compatible with one of the most popular applications out there. In short, that code cannot be removed. Devin -- Devin J. Heitmueller - Kernel Labs http://www.kernellabs.com -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [REVIEW] au0828-video.c
On 12/12/2014 04:52 PM, Devin Heitmueller wrote: No, tvtime no longer hangs if no frames arrive, so there is no need for this timeout handling. I'd strip it out, which can be done in a separate patch. Did you actually try it? Mauro tried it, not me. I'm not sure if he looked at whether the user interface is blocked when waiting for a frame. Do you have some patches to tvtime which aren't upstream? I wrote the comment in question (and added the associated code). The issue is that tvtime does *everything* in a single thread (except the recent ALSA audio work), that includes servicing the video/vbi devices as well as the user interface. That thread blocks on a DQBUF ioctl until data arrives, and thus if frames are not being delivered it will hang the entire tvtime user interface. Now you can certainly argue that is a bad design decision, but it's been that way for 15+ years, so we can't break it now. Hence why I generate dummy frames on a timeout if the decoder isn't delivering video. Unfortunately the au8522 doesn't have a free running mode (i.e. blue screen if no video), which is why most of the other devices work fine (decoders by Conexant, NXP, Trident, etc all have such functionality). Don't get me wrong - I *hate* that I had to put that timer crap in the driver, but it was necessary to be compatible with one of the most popular applications out there. In short, that code cannot be removed. Sure it can. I just tried tvtime and you are right, it blocks the GUI. But the fix is very easy as well. So now I've updated tvtime so that it timeouts and gives the GUI time to update itself. No more need for such an ugly hack in au0828. The au0828 isn't the only driver that can block, others do as well. Admittedly, they aren't very common, but they do exist. So it is much better to fix the application than adding application workarounds in the kernel. Regards, Hans -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [REVIEW] au0828-video.c
In short, that code cannot be removed. Sure it can. I just tried tvtime and you are right, it blocks the GUI. But the fix is very easy as well. So now I've updated tvtime so that it timeouts and gives the GUI time to update itself. That's a nice change to tvtime and I'm sure it will make it more robust. No more need for such an ugly hack in au0828. The au0828 isn't the only driver that can block, others do as well. Admittedly, they aren't very common, but they do exist. So it is much better to fix the application than adding application workarounds in the kernel. You're breaking the ABI. You're making a change to the kernel that causes existing applications to stop working. Sure you can make the argument that applications probably never should have expected such behavior (even if it's relied on that behavior for 15+ years). And sure, you can make a change to the application in some random git repository that avoids the issue, and that change might get sucked in to the major distributions over the next couple of years. That doesn't change the fact that you're breaking the ABI and everybody who has the existing application that updates their kernel will stop working. Please don't do this. Devin -- Devin J. Heitmueller - Kernel Labs http://www.kernellabs.com -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [REVIEW] au0828-video.c
Em Fri, 12 Dec 2014 11:46:13 -0500 Devin Heitmueller dheitmuel...@kernellabs.com escreveu: In short, that code cannot be removed. Sure it can. I just tried tvtime and you are right, it blocks the GUI. But the fix is very easy as well. So now I've updated tvtime so that it timeouts and gives the GUI time to update itself. That's a nice change to tvtime and I'm sure it will make it more robust. No more need for such an ugly hack in au0828. The au0828 isn't the only driver that can block, others do as well. Admittedly, they aren't very common, but they do exist. So it is much better to fix the application than adding application workarounds in the kernel. You're breaking the ABI. You're making a change to the kernel that causes existing applications to stop working. Sure you can make the argument that applications probably never should have expected such behavior (even if it's relied on that behavior for 15+ years). And sure, you can make a change to the application in some random git repository that avoids the issue, and that change might get sucked in to the major distributions over the next couple of years. That doesn't change the fact that you're breaking the ABI and everybody who has the existing application that updates their kernel will stop working. Please don't do this. I agree. We should not break the ABI, except if we're 100% sure that all apps that rely on the old behavior got fixed at the distros. That means that we'll need to keep holding such timeout code for years, until all distros update to a new tvtime, of course assuming that this is the only one application with such issue. With regards to tvtime, I think we need to bump version there and update it at the distros. I added myself a few patches today, in order to fix it to work with vivid driver on webcam mode. My proposal is to wait for one extra week for people to review it. As we've discussed on IRC channel, it would be good to add support for format enumeration on it, but the changes don't seem to be trivial. I'm not willing to do it, due to my lack of time, but, if someone steps up for doing that, then we can wait for those patches before bumping the version. In anycase, if everything is ok after ~1 week of waiting for tests, we can bump to version 1.0.5 and I can port the latest version to Fedora. I dunno who maintains it on other distros. Regards, Mauro -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [REVIEW] au0828-video.c
As we've discussed on IRC channel, it would be good to add support for format enumeration on it, but the changes don't seem to be trivial. I'm not willing to do it, due to my lack of time, but, if someone steps up for doing that, then we can wait for those patches before bumping the version. I agree that format enumeration will be a PITA - I looked at doing it myself a couple of years ago. Much of the problems are related to limitations in the home-grown widget toolkit that tvtime uses. I've also got patches to support HD resolutions (i.e. HDMI capture) which we use internally, but haven't felt they were worthwhile to upstream since they don't depend on the ENUM_FRAMESIZES/FRAMEINTERVALS. If somebody feels the urge to put some time into it, I can make available the patches. Devin -- Devin J. Heitmueller - Kernel Labs http://www.kernellabs.com -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
