[PATCH] More ioctls for VIA sound driver, Flash 5 now fixed

[PATCH] More ioctls for VIA sound driver, Flash 5 now fixed

[Pavel Roskin]

Hello!

Flash plugin version 5 refuses to work with the VIA 82Cxxx driver.  It
turns out that Flash uses SNDCTL_DSP_NONBLOCK on /dev/dsp, which is not
supported by the driver.

I also looked what other ioctls can be implemented easily on VIA 82Cxxx.  
There is another one - SNDCTL_DSP_GETTRIGGER.  Everything else is not 
trivial, sorry.

This patch add support for SNDCTL_DSP_NONBLOCK and SNDCTL_DSP_GETTRIGGER.
It can be found at http://www.red-bean.com/~proski/linux/via-ioctl.diff

Flash 5 plugin plays just fine after applying the patch (check e.g.  
http://wcrb.com/sparks.html)

The patch is against 2.4.13-ac2.

----------------------------------------
--- linux.orig/drivers/sound/via82cxxx_audio.c
+++ linux/drivers/sound/via82cxxx_audio.c
@@ -2800,6 +2800,11 @@ static int via_dsp_ioctl (struct inode *
 		rc = 0;
 		break;
 
+	case SNDCTL_DSP_NONBLOCK:
+		file->f_flags |= O_NONBLOCK;
+		rc = 0;
+		break;
+
 	/* obtain bitmask of device capabilities, such as mmap, full duplex, etc. */
 	case SNDCTL_DSP_GETCAPS:
 		DPRINTK ("DSP_GETCAPS\n");
@@ -2898,6 +2903,15 @@ static int via_dsp_ioctl (struct inode *
 		if (!rc && wr)
 			rc = via_dsp_ioctl_trigger (&card->ch_out, val);
 
+		break;
+
+	case SNDCTL_DSP_GETTRIGGER:
+		val = 0;
+		if ((file->f_mode & FMODE_READ) && card->ch_in.is_enabled)
+			val |= PCM_ENABLE_INPUT;
+		if ((file->f_mode & FMODE_WRITE) && card->ch_out.is_enabled)
+			val |= PCM_ENABLE_OUTPUT;
+		rc = put_user(val, (int *)arg);
 		break;
 
 	/* Enable full duplex.  Since we do this as soon as we are opened
----------------------------------------

Thanks for applying my previous patches.

P.S. Loading and unloading the driver with CONFIG_SOUND_VIA82CXXX_PROCFS
enabled blocks something in the kernel (no new process can be run).  The 
kernel prints:

de_put: deferred delete of 0
de_put: deferred delete of via

Just be aware that it can happen and that it's not related to my ioctl 
patch.  If I fix it, I'll post a separate patch.

-- 
Regards,
Pavel Roskin

Re: [PATCH] More ioctls for VIA sound driver, Flash 5 now fixed

[Jeff Garzik]

Pavel Roskin wrote:
> 
> Hello!
> 
> Flash plugin version 5 refuses to work with the VIA 82Cxxx driver.  It
> turns out that Flash uses SNDCTL_DSP_NONBLOCK on /dev/dsp, which is not
> supported by the driver.
> 
> I also looked what other ioctls can be implemented easily on VIA 82Cxxx.
> There is another one - SNDCTL_DSP_GETTRIGGER.  Everything else is not
> trivial, sorry.
> 
> This patch add support for SNDCTL_DSP_NONBLOCK and SNDCTL_DSP_GETTRIGGER.
> It can be found at http://www.red-bean.com/~proski/linux/via-ioctl.diff
> 
> Flash 5 plugin plays just fine after applying the patch (check e.g.
> http://wcrb.com/sparks.html)

Thanks, applied.

I always thought SNDCTL_DSP_NONBLOCK was stupid and never implemented
it, since the same can be accomplished via fcntl(2).  But not only this
but also some soundmodem utilities require this ioctl.  Sigh.  :)

	Jeff


-- 
Jeff Garzik      | Only so many songs can be sung
Building 1024    | with two lips, two lungs, and one tongue.
MandrakeSoft     |         - nomeansno

Re: [PATCH] More ioctls for VIA sound driver, Flash 5 now fixed

[Pavel Roskin]

Hi, Jeff!

> > Flash 5 plugin plays just fine after applying the patch (check e.g.
> > http://wcrb.com/sparks.html)
> 
> Thanks, applied.
> 
> I always thought SNDCTL_DSP_NONBLOCK was stupid and never implemented
> it, since the same can be accomplished via fcntl(2).  But not only this
> but also some soundmodem utilities require this ioctl.  Sigh.  :)

I'm more surprised that every driver should implement those ioctls
individually.  There is no common ioctl handler for non-OSS based drivers.
There is almost no code sharing between sound drivers.  As a result, every
driver comes with it's unique set of bugs and limitations :-)

I just hope that ALSA will solve this problem.  

Here's another unique limitation of of the VIA driver.  The maximal
fragment size is limited to PAGE_SIZE (scan for VIA_MAX_FRAG_SIZE).  This
is just 4k.  Flash plugin version 4 (not 5) tries to set fragment size to
8k, but doesn't check the result and plays the sound at "double speed".  
I believe that it plays the first half of every fragment.

A workaround that actually works is to replace PAGE_SIZE with 
(2*PAGE_SIZE) and PAGE_SHIFT with (PAGE_SHIFT+1).

For comparison, ymfpci.c allows fragment size up to 32k.  It also tries to 
allocate 32k first (see DMABUF_DEFAULTORDER), then 16k, 8k and finally 4k.

Perhaps the same could be done in the VIA driver.  But maybe there is some 
real reason to limit fragments to 4k?  Is there anything special with 4k 
apart from it being PAGE_SIZE on i386?

>From what I see in via_chan_buffer_init(), it tries to allocate enough
4k-sized buffers for the current chan->frag_number and chan->frag_size.  
This is very different from other drivers that try to allocate one
contiguous buffer as big as the system allows and then limit the fragment
size accordingly.

I understand that Flash 4 is obsoleted by Flash 5 that now works, but 
who knows if other programs need large fragments?  Do you think it should 
be fixed?  What do you think would be the right fix?

-- 
Regards,
Pavel Roskin

Re: [PATCH] More ioctls for VIA sound driver, Flash 5 now fixed

[Jeff Garzik]

Pavel Roskin wrote:
> 
> Hi, Jeff!
> 
> > > Flash 5 plugin plays just fine after applying the patch (check e.g.
> > > http://wcrb.com/sparks.html)
> >
> > Thanks, applied.
> >
> > I always thought SNDCTL_DSP_NONBLOCK was stupid and never implemented
> > it, since the same can be accomplished via fcntl(2).  But not only this
> > but also some soundmodem utilities require this ioctl.  Sigh.  :)
> 
> I'm more surprised that every driver should implement those ioctls
> individually.  There is no common ioctl handler for non-OSS based drivers.
> There is almost no code sharing between sound drivers.  As a result, every
> driver comes with it's unique set of bugs and limitations :-)

This is very true...  Zab, Alan, I, and others complain about this often
:)


> I just hope that ALSA will solve this problem.

Agreed.  I am hoping to finish my review of ALSA before 2.5 is
released.  That's the direction we are heading for in 2.5, but a it
needs outside review first, and at least one thing removed from their
kernel code (software rate conversion in the kernel).


> Here's another unique limitation of of the VIA driver.  The maximal
> fragment size is limited to PAGE_SIZE (scan for VIA_MAX_FRAG_SIZE).  This
> is just 4k.  Flash plugin version 4 (not 5) tries to set fragment size to
> 8k, but doesn't check the result and plays the sound at "double speed".
> I believe that it plays the first half of every fragment.
> 
> A workaround that actually works is to replace PAGE_SIZE with
> (2*PAGE_SIZE) and PAGE_SHIFT with (PAGE_SHIFT+1).
> 
> For comparison, ymfpci.c allows fragment size up to 32k.  It also tries to
> allocate 32k first (see DMABUF_DEFAULTORDER), then 16k, 8k and finally 4k.
> 
> Perhaps the same could be done in the VIA driver.  But maybe there is some
> real reason to limit fragments to 4k?  Is there anything special with 4k
> apart from it being PAGE_SIZE on i386?

I do not recall if there are any hidden limitations, but I do not think
there are.

Most of the job, I believe, would involve s/PAGE_SIZE/card->sg_buf_size/


> From what I see in via_chan_buffer_init(), it tries to allocate enough
> 4k-sized buffers for the current chan->frag_number and chan->frag_size.
> This is very different from other drivers that try to allocate one
> contiguous buffer as big as the system allows and then limit the fragment
> size accordingly.

Correct and that is intentional -- the one big buffer approach is
horrible.  If the system is heavily fragmented, one-big-alloc will fail,
and limit the total size of your audio buffer.

That is the beauty of scatter-gather hardware.  No matter how badly the
system memory is fragmented, you can usually satisfy multiple small
malloc requests, when larger requests would fail.

So, the preferred allocation algorithm would be:

	if (OSS fragment size <= PAGE_SIZE)
		allocate chan->pgtbl[] in PAGE_SIZE chunks
	else
		allocate chan->pgtbl[] in oss_frag_size chunks

Another key thing to rememeber is that pci_alloc_consistent usually
returns a -minimum- of one page, so it's useless to allocate less than
that, without switching the entire driver to the pci_pool_xxx API.


> I understand that Flash 4 is obsoleted by Flash 5 that now works, but
> who knows if other programs need large fragments?  Do you think it should
> be fixed?  What do you think would be the right fix?

I agree that the driver should support >PAGE_SIZE fragments.  And I will
gladly accept any tested patch which changes the driver to do such.  I'm
too busy at the moment to tackle it myself.

	Jeff


-- 
Jeff Garzik      | Only so many songs can be sung
Building 1024    | with two lips, two lungs, and one tongue.
MandrakeSoft     |         - nomeansno

Re: [PATCH] More ioctls for VIA sound driver, Flash 5 now fixed

[Jeff Garzik]

Jeff Garzik wrote:
> So, the preferred allocation algorithm would be:
> 
>         if (OSS fragment size <= PAGE_SIZE)
>                 allocate chan->pgtbl[] in PAGE_SIZE chunks
>         else
>                 allocate chan->pgtbl[] in oss_frag_size chunks
> 
> Another key thing to rememeber is that pci_alloc_consistent usually
> returns a -minimum- of one page, so it's useless to allocate less than
> that, without switching the entire driver to the pci_pool_xxx API.

Another limitation, I just remembered:  Each scatter-gather buffer must
be a multiple of PAGE_SIZE (actually probably PAGE_CACHE_SIZE), in order
for mmap(2) support (via_mm_nopage) to work properly.

I would also like to point out that mmap support via
vm_operations_struct::nopage is also unique and new, and was suggested
by Linus as a much better mmap(2) approach than other drivers, which
using remap_page_range [a method which requires the one-big-buffer
allocation approach].

	Jeff


-- 
Jeff Garzik      | Only so many songs can be sung
Building 1024    | with two lips, two lungs, and one tongue.
MandrakeSoft     |         - nomeansno