cx88: fix unexpected video resize when setting tv norm
Commit Message
On Sat, 10 Jan 2009, Marton Balint wrote:
> Cx88_set_tvnorm sets the size of the video to fixed 320x240. This is ugly at
> least, but also can cause problems, if it happens during an active video
> transfer. With this patch, cx88_set_scale will save the last requested video
> size, and cx88_set_tvnorm will scale the video to this size.
>
> diff -r 985ecd81d993 -r 571b3176dc82 linux/drivers/media/video/cx88/cx88.h
> @@ -352,6 +352,9 @@
> u32 input;
> u32 astat;
> u32 use_nicam;
> + unsigned int last_width;
> + unsigned int last_height;
> + enum v4l2_field last_field;
Instead of adding these extra fields to the core, maybe it would be better
to just add w/h/field as arguments to set_tvnorm? I have a patch to do
this, but there are still problems.
The allowable sizes depends on the video norm. If you select 720x576 in
PAL and then change the norm to NTSC bad things will happen if the driver
tries to maintain more than 480 lines. So cx88_set_scale() will happily
program bogus register values on size change.
cx88_set_tvnorm() would need to check if the current size can be maintained
in the new norm and if it's not, change it to something valid (what?). Or
maybe the S_STD ioctl handler should adjust the size to something valid?
What does V4L2 say about what should happen if the current format will no
longer be valid after a norm change? Should the norm change fail? Should
the format be adjusted to one that is valid? The norm is per device but
the format is per file handle, so would changing the norm on one file
handle modify the format of a different open file handle? That doesn't
seem right. But, v4l2 seems require that you aren't allowed to set an
invalid format, so getting an invalid format via a norm change seems wrong
too.
Changing norms during capture has more problems. I'm not sure if v4l2 even
allows it. Even if allowed, I don't think the cx88 driver should try to
support it.
The norm change code will immediately program a bunch of register values
when the norm is set. These could easily screw up current video activity.
Suppose the cx88 is in the middle of capturing a 576 line PAL frame and the
norm is changed to NTSC. How is that supposed to be handled?
In my patch, setting the tvnorm keeps the file handle's current size. This
won't work during capture as the cx88's scalers are programmed on a
frame-by-frame basis and the current frame being captured might not be the
same size as the file handle which tried to change the norm.
I think there is also a race in your patch, as the call to cx88_set_scale()
when a frame is queued isn't protected against the tvnorm ioctl. It might
be possible to fix that by grabbing the queue spinlock before changing the
norm. Still, I don't think the code the programs the registers for a norm
change is designed to be safe to call during capture.
So I think the best thing would be to have S_STD return -EBUSY if there is
an ongoing capture. Maybe even have v4l2-dev take care of that if
possible.
---
--
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
Comments
On Sat, 10 Jan 2009, Trent Piepho wrote:
> On Sat, 10 Jan 2009, Marton Balint wrote:
> > Cx88_set_tvnorm sets the size of the video to fixed 320x240. This is ugly at
> > least, but also can cause problems, if it happens during an active video
> > transfer. With this patch, cx88_set_scale will save the last requested video
> > size, and cx88_set_tvnorm will scale the video to this size.
> >
> > diff -r 985ecd81d993 -r 571b3176dc82 linux/drivers/media/video/cx88/cx88.h
> > @@ -352,6 +352,9 @@
> > u32 input;
> > u32 astat;
> > u32 use_nicam;
> > + unsigned int last_width;
> > + unsigned int last_height;
> > + enum v4l2_field last_field;
>
> Instead of adding these extra fields to the core, maybe it would be better
> to just add w/h/field as arguments to set_tvnorm? I have a patch to do
> this, but there are still problems.
I think you're right, it's probably better that way.
> Changing norms during capture has more problems. I'm not sure if v4l2 even
> allows it. Even if allowed, I don't think the cx88 driver should try to
> support it.
What the other drivers do?
> So I think the best thing would be to have S_STD return -EBUSY if there is
> an ongoing capture. Maybe even have v4l2-dev take care of that if
> possible.
It sounds reasonable. As a special case, changing the norm to the
current norm should be allowed, or not? Mplayer will print out error
messages, if it's not allowed.
Regards,
Marton.
--
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 Sat, 10 Jan 2009, Trent Piepho wrote:
> On Sat, 10 Jan 2009, Marton Balint wrote:
> > Cx88_set_tvnorm sets the size of the video to fixed 320x240. This is ugly at
> > least, but also can cause problems, if it happens during an active video
> > transfer. With this patch, cx88_set_scale will save the last requested video
> > size, and cx88_set_tvnorm will scale the video to this size.
>
> Instead of adding these extra fields to the core, maybe it would be better
> to just add w/h/field as arguments to set_tvnorm? I have a patch to do
> this, but there are still problems.
>
> The allowable sizes depends on the video norm. If you select 720x576 in
> PAL and then change the norm to NTSC bad things will happen if the driver
> tries to maintain more than 480 lines. So cx88_set_scale() will happily
> program bogus register values on size change.
>
> cx88_set_tvnorm() would need to check if the current size can be maintained
> in the new norm and if it's not, change it to something valid (what?). Or
> maybe the S_STD ioctl handler should adjust the size to something valid?
>
> What does V4L2 say about what should happen if the current format will no
> longer be valid after a norm change? Should the norm change fail? Should
> the format be adjusted to one that is valid? The norm is per device but
> the format is per file handle, so would changing the norm on one file
> handle modify the format of a different open file handle? That doesn't
> seem right. But, v4l2 seems require that you aren't allowed to set an
> invalid format, so getting an invalid format via a norm change seems wrong
> too.
>
> Changing norms during capture has more problems. I'm not sure if v4l2 even
> allows it. Even if allowed, I don't think the cx88 driver should try to
> support it.
>
> The norm change code will immediately program a bunch of register values
> when the norm is set. These could easily screw up current video activity.
> Suppose the cx88 is in the middle of capturing a 576 line PAL frame and the
> norm is changed to NTSC. How is that supposed to be handled?
>
> In my patch, setting the tvnorm keeps the file handle's current size. This
> won't work during capture as the cx88's scalers are programmed on a
> frame-by-frame basis and the current frame being captured might not be the
> same size as the file handle which tried to change the norm.
>
> I think there is also a race in your patch, as the call to cx88_set_scale()
> when a frame is queued isn't protected against the tvnorm ioctl. It might
> be possible to fix that by grabbing the queue spinlock before changing the
> norm. Still, I don't think the code the programs the registers for a norm
> change is designed to be safe to call during capture.
>
> So I think the best thing would be to have S_STD return -EBUSY if there is
> an ongoing capture. Maybe even have v4l2-dev take care of that if
> possible.
The status of this patch has changed to "Changes Requested" in
patchwork, but it's not obvious to me what changes are needed exactly.
Yes, in the comments quite a few questions came up, but we haven't
decided the correct course of action for good, and the patch also makes
sense in it's current form.
Regards,
Marton
--
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 Thu, 29 Jan 2009, Marton Balint wrote:
> The status of this patch has changed to "Changes Requested" in
> patchwork, but it's not obvious to me what changes are needed exactly.
> Yes, in the comments quite a few questions came up, but we haven't
> decided the correct course of action for good, and the patch also makes
> sense in it's current form.
The most serious problem with the patch is that the current image size may
not be valid after changing norms. The driver and v4l2 aren't designed to
allow an invalid image size to be selected, yet this patch would allow that
to happen.
--
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
@@ -1058,10 +1058,12 @@ static int vidioc_s_tuner (struct file *
static int vidioc_s_std (struct file *file, void *priv, v4l2_std_id *id)
{
- struct cx88_core *core = ((struct cx8802_fh *)priv)->dev->core;
+ struct cx8802_fh *fh = priv;
+ struct cx88_core *core = fh->dev->core;
mutex_lock(&core->lock);
- cx88_set_tvnorm(core,*id);
+ cx88_set_tvnorm(core, *id, fh->dev->width, fh->dev->height,
+ fh->mpegq.field);
mutex_unlock(&core->lock);
return 0;
}
@@ -1370,7 +1372,7 @@ static int cx8802_blackbird_probe(struct
#if 1
mutex_lock(&dev->core->lock);
// init_controls(core);
- cx88_set_tvnorm(core,core->tvnorm);
+ cx88_set_tvnorm(core, core->tvnorm, 320, 240, V4L2_FIELD_INTERLACED);
cx88_video_mux(core,0);
mutex_unlock(&dev->core->lock);
#endif
@@ -905,7 +905,9 @@ static int set_tvaudio(struct cx88_core
-int cx88_set_tvnorm(struct cx88_core *core, v4l2_std_id norm)
+int cx88_set_tvnorm(struct cx88_core *core, v4l2_std_id norm,
+ unsigned int width, unsigned int height,
+ enum v4l2_field field)
{
u32 fsc8;
u32 adc_clock;
@@ -1014,7 +1016,7 @@ int cx88_set_tvnorm(struct cx88_core *co
cx_write(MO_VBI_PACKET, (10<<11) | norm_vbipack(norm));
// this is needed as well to set all tvnorm parameter
- cx88_set_scale(core, 320, 240, V4L2_FIELD_INTERLACED);
+ cx88_set_scale(core, width, height, field);
// audio
set_tvaudio(core);
@@ -1490,10 +1490,11 @@ static int vidioc_streamoff(struct file
static int vidioc_s_std (struct file *file, void *priv, v4l2_std_id *tvnorms)
{
- struct cx88_core *core = ((struct cx8800_fh *)priv)->dev->core;
+ struct cx8800_fh *fh = priv;
+ struct cx88_core *core = fh->dev->core;
mutex_lock(&core->lock);
- cx88_set_tvnorm(core,*tvnorms);
+ cx88_set_tvnorm(core, *tvnorms, fh->width, fh->height, fh->vidq.field);
mutex_unlock(&core->lock);
return 0;
@@ -2221,7 +2222,7 @@ static int __devinit cx8800_initdev(stru
/* initial device configuration */
mutex_lock(&core->lock);
- cx88_set_tvnorm(core,core->tvnorm);
+ cx88_set_tvnorm(core, core->tvnorm, 320, 240, V4L2_FIELD_INTERLACED);
init_controls(core);
cx88_video_mux(core,0);
mutex_unlock(&core->lock);
@@ -594,7 +594,9 @@ extern void cx88_sram_channel_dump(struc
extern int cx88_set_scale(struct cx88_core *core, unsigned int width,
unsigned int height, enum v4l2_field field);
-extern int cx88_set_tvnorm(struct cx88_core *core, v4l2_std_id norm);
+extern int cx88_set_tvnorm(struct cx88_core *core, v4l2_std_id norm,
+ unsigned int width, unsigned int height,
+ enum v4l2_field field);
extern struct video_device *cx88_vdev_init(struct cx88_core *core,
struct pci_dev *pci,