Allocating DMA buffer for non-PCM

Allocating DMA buffer for non-PCM

[Adrian Knoth]

Hi!

Would you say that the following is the proper way to allocate a DMA
buffer used to hold level data?

struct hdspm {
	struct pci_dev *pci;	/* and an pci info */
	struct snd_dma_buffer dmaLevelBuffer;
	u32 *level_buffer;	/* suitably aligned address */
    [..]
}

	/* allocate level buffer */
	err = snd_dma_alloc_pages(SNDRV_DMA_TYPE_DEV_SG,
			snd_dma_pci_data(hdspm->pci),
			MADIFX_LEVEL_BUFFER_SIZE, &hdspm->dmaLevelBuffer);
	if (err < 0) {
    /* error */ [..]
    }

    hdspm->level_buffer = snd_sgbuf_get_ptr(&(hdspm->dmaLevelBuffer), 0);

	memset(hdspm->level_buffer, 0, MADIFX_LEVEL_BUFFER_SIZE);


This used to work on my development machine (kernel 3.6.x), but now a
kernel 3.2.0 user reports a NULL pointer dereference of
hdspm->level_buffer, so apparently, snd_sgbuf_get_ptr() returned NULL
for him.

How could this possibly happen? Am I missing something? Better use
SNDRV_DMA_TYPE_DEV instead?



TIA

Re: Allocating DMA buffer for non-PCM

[Takashi Iwai]

At Thu, 14 Feb 2013 18:03:53 +0100,
Adrian Knoth wrote:
> 
> Hi!
> 
> Would you say that the following is the proper way to allocate a DMA
> buffer used to hold level data?
> 
> struct hdspm {
> 	struct pci_dev *pci;	/* and an pci info */
> 	struct snd_dma_buffer dmaLevelBuffer;
> 	u32 *level_buffer;	/* suitably aligned address */
>     [..]
> }
> 
> 	/* allocate level buffer */
> 	err = snd_dma_alloc_pages(SNDRV_DMA_TYPE_DEV_SG,
> 			snd_dma_pci_data(hdspm->pci),
> 			MADIFX_LEVEL_BUFFER_SIZE, &hdspm->dmaLevelBuffer);
> 	if (err < 0) {
>     /* error */ [..]
>     }
> 
>     hdspm->level_buffer = snd_sgbuf_get_ptr(&(hdspm->dmaLevelBuffer), 0);

hdspm->level_buffer = (u32*)hdspm.dmaLevelBuffer.area;

> 	memset(hdspm->level_buffer, 0, MADIFX_LEVEL_BUFFER_SIZE);
> 
> 
> This used to work on my development machine (kernel 3.6.x), but now a
> kernel 3.2.0 user reports a NULL pointer dereference of
> hdspm->level_buffer, so apparently, snd_sgbuf_get_ptr() returned NULL
> for him.

What's level_buffer?  Is a SG-buffer for the audio stream?

> How could this possibly happen?

A wrong usage :)


Takashi

> Am I missing something? Better use
> SNDRV_DMA_TYPE_DEV instead?
> 
> 
> 
> TIA
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
> 

Re: Allocating DMA buffer for non-PCM

[Adrian Knoth]

On 02/14/2013 06:14 PM, Takashi Iwai wrote:

>> 	/* allocate level buffer */
>> 	err = snd_dma_alloc_pages(SNDRV_DMA_TYPE_DEV_SG,
>> 			snd_dma_pci_data(hdspm->pci),
>> 			MADIFX_LEVEL_BUFFER_SIZE, &hdspm->dmaLevelBuffer);
>> 	if (err < 0) {
>>     /* error */ [..]
>>     }
>>
>>     hdspm->level_buffer = snd_sgbuf_get_ptr(&(hdspm->dmaLevelBuffer), 0);
> 
> hdspm->level_buffer = (u32*)hdspm.dmaLevelBuffer.area;

Looks good, TNX.

>> This used to work on my development machine (kernel 3.6.x), but now a
>> kernel 3.2.0 user reports a NULL pointer dereference of
>> hdspm->level_buffer, so apparently, snd_sgbuf_get_ptr() returned NULL
>> for him.
> What's level_buffer?

The card does hardware metering and stores RMS/peak values in a DMA
buffer. I want to later pass this to userspace to show signal levels,
either via memcpy()ing the DMA buffer or maybe via mmap(). But since I
no longer have access to such a card, work on this is on halt atm.

> Is a SG-buffer for the audio stream?

The audio buffers use

	     snd_pcm_lib_preallocate_pages_for_all(pcm,
						   SNDRV_DMA_TYPE_DEV_SG,
						   snd_dma_pci_data(hdspm->pci),


so they're SG, yes.

>> How could this possibly happen?
> A wrong usage :)

I thought so, that's why I was asking. ;)



Cheers and thanks again

Re: Allocating DMA buffer for non-PCM

[Clemens Ladisch]

Adrian Knoth wrote:
> Would you say that the following is the proper way to allocate a DMA
> buffer used to hold level data?
>
> 	err = snd_dma_alloc_pages(SNDRV_DMA_TYPE_DEV_SG,
> 			snd_dma_pci_data(hdspm->pci),
> 			MADIFX_LEVEL_BUFFER_SIZE, &hdspm->dmaLevelBuffer);
>
>     hdspm->level_buffer = snd_sgbuf_get_ptr(&(hdspm->dmaLevelBuffer), 0);

I don't see the code asking for the address of the second page, so I
guess there isn't one.  But then you don't need SG in the first place.

> Better use SNDRV_DMA_TYPE_DEV instead?

Yes.


Regards,
Clemens

Re: Allocating DMA buffer for non-PCM

[Adrian Knoth]

On Thu, Feb 14, 2013 at 06:46:30PM +0100, Clemens Ladisch wrote:

> > Would you say that the following is the proper way to allocate a DMA
> > buffer used to hold level data?
> >
> > 	err = snd_dma_alloc_pages(SNDRV_DMA_TYPE_DEV_SG,
> > 			snd_dma_pci_data(hdspm->pci),
> > 			MADIFX_LEVEL_BUFFER_SIZE, &hdspm->dmaLevelBuffer);
> >
> >     hdspm->level_buffer = snd_sgbuf_get_ptr(&(hdspm->dmaLevelBuffer), 0);
> I don't see the code asking for the address of the second page, so I
> guess there isn't one.

Exactly.

Just to be sure: Takashi has recommended to use
(u32*)dmaLevelBuffer.area. Even in the case of SG buffers, is this
virtual address continuous and safe for memset?


Cheers

-- 
mail: adi@thur.de  	http://adi.thur.de	PGP/GPG: key via keyserver

Re: Allocating DMA buffer for non-PCM

[Clemens Ladisch]

Adrian Knoth wrote:
> On Thu, Feb 14, 2013 at 06:46:30PM +0100, Clemens Ladisch wrote:
>>> Would you say that the following is the proper way to allocate a DMA
>>> buffer used to hold level data?
>>>
>>> 	err = snd_dma_alloc_pages(SNDRV_DMA_TYPE_DEV_SG,
>>> 			snd_dma_pci_data(hdspm->pci),
>>> 			MADIFX_LEVEL_BUFFER_SIZE, &hdspm->dmaLevelBuffer);
>>>
>>>     hdspm->level_buffer = snd_sgbuf_get_ptr(&(hdspm->dmaLevelBuffer), 0);
>>
>> I don't see the code asking for the address of the second page, so I
>> guess there isn't one.
>
> Exactly.

So MADIFX_LEVEL_BUFFER_SIZE is guaranteed to not exceed the page size
on all architectures?

> Just to be sure: Takashi has recommended to use
> (u32*)dmaLevelBuffer.area. Even in the case of SG buffers, is this
> virtual address continuous and safe for memset?

Yes, the SG buffer's pages are vmap()ed there.


Regards,
Clemens

Re: Allocating DMA buffer for non-PCM

[Adrian Knoth]

On 02/14/2013 07:01 PM, Clemens Ladisch wrote:

>> On Thu, Feb 14, 2013 at 06:46:30PM +0100, Clemens Ladisch wrote:
>>>> Would you say that the following is the proper way to allocate a DMA
>>>> buffer used to hold level data?
>>>>
>>>> 	err = snd_dma_alloc_pages(SNDRV_DMA_TYPE_DEV_SG,
>>>> 			snd_dma_pci_data(hdspm->pci),
>>>> 			MADIFX_LEVEL_BUFFER_SIZE, &hdspm->dmaLevelBuffer);
>>>>
>>>>     hdspm->level_buffer = snd_sgbuf_get_ptr(&(hdspm->dmaLevelBuffer), 0);
>>>
>>> I don't see the code asking for the address of the second page, so I
>>> guess there isn't one.
>>
>> Exactly.
> 
> So MADIFX_LEVEL_BUFFER_SIZE is guaranteed to not exceed the page size
> on all architectures?

Absolutely not, it's known to be multiples of the page size, but the
purpose of hdspm->level_buffer is to hold the continuous virtual
address, so apparently, I was using the wrong getter function in the
first place (I was actually looking for .area).

The code for accessing the other pages was omitted for the sake of
simplicity, but since you've asked, here it is:

   	/* Fill level page table */
	for (i = 0; i < MADIFX_NUM_LEVEL_PAGES; i++) {
		levelPageTable[i] = snd_sgbuf_get_addr(&(hdspm->dmaLevelBuffer),
				i * MADIFX_HARDWARE_PAGE_SIZE);

	}

	/* Write level page table to device */
	lpti = (MADIFX == hdspm->io_type) ? MADIFX_LPTI_HMFX :
		MADIFX_LPTI_MFXT;

	for (i = 0; i < MADIFX_NUM_LEVEL_PAGES; i++) {
		madifx_write(hdspm, MADIFX_PAGE_ADDRESS_LIST + (4 * (lpti + i)),
				levelPageTable[i]);
	}



Cheers

Re: Allocating DMA buffer for non-PCM

[Takashi Iwai]

At Thu, 14 Feb 2013 18:26:53 +0100,
Adrian Knoth wrote:
> 
> On 02/14/2013 06:14 PM, Takashi Iwai wrote:
> 
> >> 	/* allocate level buffer */
> >> 	err = snd_dma_alloc_pages(SNDRV_DMA_TYPE_DEV_SG,
> >> 			snd_dma_pci_data(hdspm->pci),
> >> 			MADIFX_LEVEL_BUFFER_SIZE, &hdspm->dmaLevelBuffer);
> >> 	if (err < 0) {
> >>     /* error */ [..]
> >>     }
> >>
> >>     hdspm->level_buffer = snd_sgbuf_get_ptr(&(hdspm->dmaLevelBuffer), 0);
> > 
> > hdspm->level_buffer = (u32*)hdspm.dmaLevelBuffer.area;
> 
> Looks good, TNX.
> 
> >> This used to work on my development machine (kernel 3.6.x), but now a
> >> kernel 3.2.0 user reports a NULL pointer dereference of
> >> hdspm->level_buffer, so apparently, snd_sgbuf_get_ptr() returned NULL
> >> for him.
> > What's level_buffer?
> 
> The card does hardware metering and stores RMS/peak values in a DMA
> buffer. I want to later pass this to userspace to show signal levels,
> either via memcpy()ing the DMA buffer or maybe via mmap(). But since I
> no longer have access to such a card, work on this is on halt atm.
> 
> > Is a SG-buffer for the audio stream?
> 
> The audio buffers use
> 
> 	     snd_pcm_lib_preallocate_pages_for_all(pcm,
> 						   SNDRV_DMA_TYPE_DEV_SG,
> 						   snd_dma_pci_data(hdspm->pci),
> 
> 
> so they're SG, yes.

But isn't the level meter buffer a single page?  If the buffer is only
for peak meters, it can't be that big even for multi-channel cards.
If so, SNDRV_DMA_TYPE_DEV_SG is utterly nonsense.  Otherwise, if it's
a SG buffer, you'll have to some code to set up TLB in anyway.


Takashi

Re: Allocating DMA buffer for non-PCM

[Takashi Iwai]

At Thu, 14 Feb 2013 19:46:39 +0100,
Adrian Knoth wrote:
> 
> On 02/14/2013 07:01 PM, Clemens Ladisch wrote:
> 
> >> On Thu, Feb 14, 2013 at 06:46:30PM +0100, Clemens Ladisch wrote:
> >>>> Would you say that the following is the proper way to allocate a DMA
> >>>> buffer used to hold level data?
> >>>>
> >>>> 	err = snd_dma_alloc_pages(SNDRV_DMA_TYPE_DEV_SG,
> >>>> 			snd_dma_pci_data(hdspm->pci),
> >>>> 			MADIFX_LEVEL_BUFFER_SIZE, &hdspm->dmaLevelBuffer);
> >>>>
> >>>>     hdspm->level_buffer = snd_sgbuf_get_ptr(&(hdspm->dmaLevelBuffer), 0);
> >>>
> >>> I don't see the code asking for the address of the second page, so I
> >>> guess there isn't one.
> >>
> >> Exactly.
> > 
> > So MADIFX_LEVEL_BUFFER_SIZE is guaranteed to not exceed the page size
> > on all architectures?
> 
> Absolutely not, it's known to be multiples of the page size, but the
> purpose of hdspm->level_buffer is to hold the continuous virtual
> address, so apparently, I was using the wrong getter function in the
> first place (I was actually looking for .area).

OK, then disregard my previous comment.  The SG-buffer is the right
thing.

FWIW, snd_sgbuf_get_ptr() return the pointer of the particular page.
If you access only the data in the page size, it'll work even with
that.


Takashi

> The code for accessing the other pages was omitted for the sake of
> simplicity, but since you've asked, here it is:
> 
>    	/* Fill level page table */
> 	for (i = 0; i < MADIFX_NUM_LEVEL_PAGES; i++) {
> 		levelPageTable[i] = snd_sgbuf_get_addr(&(hdspm->dmaLevelBuffer),
> 				i * MADIFX_HARDWARE_PAGE_SIZE);
> 
> 	}
> 
> 	/* Write level page table to device */
> 	lpti = (MADIFX == hdspm->io_type) ? MADIFX_LPTI_HMFX :
> 		MADIFX_LPTI_MFXT;
> 
> 	for (i = 0; i < MADIFX_NUM_LEVEL_PAGES; i++) {
> 		madifx_write(hdspm, MADIFX_PAGE_ADDRESS_LIST + (4 * (lpti + i)),
> 				levelPageTable[i]);
> 	}
> 
> 
> 
> Cheers
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
>