Opened 7 years ago

Closed 5 years ago

#9074 closed bug (fixed)

ICE1712 produces sinusoidal noise with 4+gb of ram

Reported by: SeanCollins Owned by: jerl1
Priority: normal Milestone: R1
Component: Drivers/Audio/ice1712 Version: R1/Development
Keywords: Cc:
Blocked By: Blocking:
Has a Patch: yes Platform: All

Description

ICE1712 produces sinusoidal noise with 4+gb of ram, no useable audio output either. Just a steady sinusoid of noise at a constant pitch. right around 4000hz "no I didn't measure", totally fine if I disable above 4gb of ram however. Changing the output rate from 44.1khz to 96khz does effect the pitch of the sound and its rate.

latest nightlys going back since the introduction of the driver afaik. 44678 gcc2h. No data in log indicating a problem either.Could be good to include this in the release notes as well. No other system instability noted with this occurnce, just the noise and only effects the ice1712 driver, the hda driver & hardware on this machine operates just fine with 8gb of ram.

Attachments (6)

syslog (526.9 KB ) - added by SeanCollins 7 years ago.
0001-Fix-3079-and-9074-Memory-alocation-beyond-256MB.patch (2.8 KB ) - added by jerl1 5 years ago.
0001-Fix-3079-and-9074-Memory-alocation-beyond-256MB.2.patch (3.0 KB ) - added by jerl1 5 years ago.
ticket_9074.patch (1.6 KB ) - added by jerl1 5 years ago.
0001-ice1712-Refactoring.patch (180.1 KB ) - added by jerl1 5 years ago.
0002-Fix-3079-and-9074-Memory-alocation-beyond-256MB.patch (2.5 KB ) - added by jerl1 5 years ago.

Download all attachments as: .zip

Change History (29)

comment:1 by korli, 7 years ago

Please post a syslog. The code already uses the flag B_32_BIT_CONTIGUOUS for create_area calls().

comment:2 by SeanCollins, 7 years ago

attaching syslog, first boot with 8gb or ram enabled, second boot with 4gb of ram enabled.

by SeanCollins, 7 years ago

Attachment: syslog added

comment:3 by SeanCollins, 7 years ago

The recent changes in the vm memory manager have had impact on the output noise, but I still get noise when using 4gb+ ram 44810hrev. previously it was a raw sinusoid, now with disk acess i get a dot matrix printer head travel like sound

Last edited 7 years ago by SeanCollins (previous) (diff)

comment:4 by korli, 7 years ago

Could you please enable verbosity here and provide a new syslog? Dunno whether there is anything to see but it could be useful.

comment:5 by jerl1, 5 years ago

Has a Patch: set

comment:6 by jerl1, 5 years ago

Resolution: fixed
Status: newclosed

ICE1712 chipset is not able to use DMA with address beyond 256MB

comment:7 by jerl1, 5 years ago

Resolution: fixed
Status: closedreopened

comment:8 by jerl1, 5 years ago

Resolution: fixed
Status: reopenedclosed

comment:9 by luroh, 5 years ago

Resolution: fixed
Status: closedreopened

Thanks for the patch. Let's keep the ticket open for now, it can be closed once the patch has been reviewed, possibly iterated upon and eventually committed to the source repository.

comment:10 by pulkomandy, 5 years ago

Hi,

#if B_HAIKU_PHYSICAL_BITS > 28 

There is no need to check for this, we only support 32 and 64 bit addressing.

I will let others review the patch since I'm not sure we want to introduce constants like this for each possible constraint required by each device, so I would say we need a more generic mechanism.

in reply to:  10 comment:11 by korli, 5 years ago

Replying to pulkomandy:

I will let others review the patch since I'm not sure we want to introduce constants like this for each possible constraint required by each device, so I would say we need a more generic mechanism.

Agreed. For instance, emuxki would need 0-2GB only (31 bits).

in reply to:  10 comment:12 by jerl1, 5 years ago

Replying to pulkomandy:

#if B_HAIKU_PHYSICAL_BITS > 28 

It was a copy and paste of the previous 'case' with change all '32' to '28'

I will let others review the patch since I'm not sure we want to introduce constants like this for each possible constraint required by each device, so I would say we need a more generic mechanism.

I do agree it's clearly not the best way to do it. But it means changing the signature of create_area.

comment:13 by pulkomandy, 5 years ago

I think it's possible to do this without changing the signature. We could introduce a B_MAX_ADDRESS flag with an effect reverse of B_BASE_ADDRESS: the passed address would be the maximal allowed value for base address + size. This does not need any ABI change and allows arbitrary address limits.

comment:14 by korli, 5 years ago

Good idea. I would rephrase to: the passed address becomes the start address of the address space window to exclude for the allocated area. I find B_MAX_ADDRESS confusing because the maximum isn't actually allowed.

comment:15 by axeld, 5 years ago

How likely is it to have arbitrary limits? Why not just introduce a flag like B_28_BIT_ADDRESS_ONLY? If arbitrary limits are actually required, I'd be for something like B_ADDRESS_MASK to specify it.

comment:16 by jerl1, 5 years ago

Something like the 2nd patch?

comment:17 by pulkomandy, 5 years ago

I would have used the other parameter to create area, which is currently B_ANY_KERNEL_ADDRESS. There already is a B_BASE_ADDRESS there, and I think it would make sense to use a B_MAX_ADDRESS (in combination with the standard B_CONTIGUOUS flag).

@axeld: as mentionned, some hardware needs a 32-bit limit, for which we already had a specific flag. This card needs 28bits, emuxki needs 31bits. We also have a flag for 24 bits (for which we have another specific flag, B_LOMEM) and some old ISA stuff with DMA needs 16bits or 20bits (which we don't have support for yet). We could have everything use B_LOMEM but this is putting unneeded memory pressure on this rather small address space, so I think a generic solution at create_area level would be more helpful.

comment:18 by bonefish, 5 years ago

Is an API change necessary at all? The driver could just use vm_create_anonymous_area() and pass respective physical_address_restrictions.

in reply to:  18 comment:19 by korli, 5 years ago

Replying to bonefish:

Is an API change necessary at all? The driver could just use vm_create_anonymous_area() and pass respective physical_address_restrictions.

Or this one: create_area_etc(). There is though this comment next to it: private kernel only extension (should be moved somewhere else) http://grok.bikemonkey.org/source/xref/haiku/headers/private/kernel/vm/vm.h#78

by jerl1, 5 years ago

Attachment: ticket_9074.patch added

comment:20 by jerl1, 5 years ago

Is patch "ticket_9074.patch" correct, I mean with the parameter. It's working I'm having sound but, as the function is not documented I don't know if i passed the correct values for all parameters

If this is ok, I have a patch to apply before this one for some refactoring, change code into C++, and warning build fixes for x86 and x86_64 compilation.

Last edited 5 years ago by jerl1 (previous) (diff)

by jerl1, 5 years ago

comment:21 by jerl1, 5 years ago

I've added 2 patchs:

  • the first one for refactoring the code
  • the second one for fixing the bug

If you can review them and commit them

Regards

comment:22 by korli, 5 years ago

Applied in hrev48854 with a few changes in the second patch: use B_ANY_KERNEL_ADDRESS, B_SYSTEM_TEAM. BTW a lot of variable naming isn't correct with the coding style since the move to C++.

comment:23 by waddlesplash, 5 years ago

Resolution: fixed
Status: reopenedclosed
Note: See TracTickets for help on using tickets.