From owner-freebsd-bugs@FreeBSD.ORG Mon Aug 15 09:20:12 2005 Return-Path: X-Original-To: freebsd-bugs@hub.freebsd.org Delivered-To: freebsd-bugs@hub.freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id B3EF516A41F for ; Mon, 15 Aug 2005 09:20:12 +0000 (GMT) (envelope-from gnats@FreeBSD.org) Received: from freefall.freebsd.org (freefall.freebsd.org [216.136.204.21]) by mx1.FreeBSD.org (Postfix) with ESMTP id 69C2943D49 for ; Mon, 15 Aug 2005 09:20:12 +0000 (GMT) (envelope-from gnats@FreeBSD.org) Received: from freefall.freebsd.org (gnats@localhost [127.0.0.1]) by freefall.freebsd.org (8.13.3/8.13.3) with ESMTP id j7F9KCH2092044 for ; Mon, 15 Aug 2005 09:20:12 GMT (envelope-from gnats@freefall.freebsd.org) Received: (from gnats@localhost) by freefall.freebsd.org (8.13.3/8.13.1/Submit) id j7F9KBMn092043; Mon, 15 Aug 2005 09:20:11 GMT (envelope-from gnats) Date: Mon, 15 Aug 2005 09:20:11 GMT Message-Id: <200508150920.j7F9KBMn092043@freefall.freebsd.org> To: freebsd-bugs@FreeBSD.org From: Ariff Abdullah Cc: Subject: Re: kern/84728: [sound] [patch] ac97 broken mixing capabilities checking X-BeenThere: freebsd-bugs@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list Reply-To: Ariff Abdullah List-Id: Bug reports List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 15 Aug 2005 09:20:12 -0000 The following reply was made to PR kern/84728; it has been noted by GNATS. From: Ariff Abdullah To: Michael Seyfert Cc: bug-followup@FreeBSD.org, Alexander@Leidinger.net, mathew.kanner@gmail.com, davidxu@FreeBSD.org Subject: Re: kern/84728: [sound] [patch] ac97 broken mixing capabilities checking Date: Mon, 15 Aug 2005 17:16:08 +0800 On Mon, 15 Aug 2005 06:59:29 +0000 Michael Seyfert wrote: > All I really needed was the check for the number of bits.. I'm not > sure what the other code is for in that patch, but > ac97.c.head.FINAL.diff seems to be working. > It's a balance between buggy codecs, different ac97 revisions (2.1/2.2/2.3), different register/bit offset (you cannot simply assume that every mixer register is 6bit long, or you end up overflowing it). Anyway, thanks! I assume *this* should be the true final version! (I'm CCing this to other people as well, especially David Xu for his 'used to be Creative EV1938 but its no longer true' for further validation. -- DIFF BEGIN -- --- sys/dev/sound/pcm/ac97.c.orig Sun Jul 31 22:28:31 2005 +++ sys/dev/sound/pcm/ac97.c Mon Aug 15 15:43:24 2005 @@ -118,6 +118,11 @@ { 0x57454300, "Winbond" }, { 0x574d4c00, "Wolfson" }, { 0x594d4800, "Yamaha" }, + /* + * XXX This is a fluke, really! The real vendor + * should be SigmaTel, not this! This should be + * removed someday! + */ { 0x01408300, "Creative" }, { 0x00000000, NULL } }; @@ -211,6 +216,11 @@ { 0x594d4800, 0x00, 0, "YMF743", 0 }, { 0x594d4802, 0x00, 0, "YMF752", 0 }, { 0x594d4803, 0x00, 0, "YMF753", 0 }, + /* + * XXX This is a fluke, really! The real codec + * should be STAC9704, not this! This should be + * removed someday! + */ { 0x01408384, 0x00, 0, "EV1938", 0 }, { 0, 0, 0, NULL, 0 } }; @@ -283,6 +293,21 @@ u_int16_t ac97_rdcd(struct ac97_info *codec, int reg) { + if (codec->flags & AC97_F_RDCD_BUG) { + u_int16_t i[2], j = 100; + + i[0] = AC97_READ(codec->methods, codec->devinfo, reg); + i[1] = AC97_READ(codec->methods, codec->devinfo, reg); + while (i[0] != i[1] && j) + i[j-- & 1] = AC97_READ(codec->methods, codec->devinfo, reg); +#if 0 + if (j < 100) { + device_printf(codec->dev, "%s(): Inconsistent register value at" + " 0x%08x (retry: %d)\n", __func__, reg, 100 - j); + } +#endif + return i[!(j & 1)]; + } return AC97_READ(codec->methods, codec->devinfo, reg); } @@ -452,14 +477,16 @@ */ snd_mtxlock(codec->lock); if (e->mask) { - int cur = ac97_rdcd(codec, e->reg); + int cur = ac97_rdcd(codec, reg); val |= cur & ~(mask); } ac97_wrcd(codec, reg, val); snd_mtxunlock(codec->lock); return left | (right << 8); } else { - /* printf("ac97_setmixer: reg=%d, bits=%d, enable=%d\n", e->reg, e->bits, e->enable); */ +#if 0 + printf("ac97_setmixer: reg=%d, bits=%d, enable=%d\n", e->reg, e->bits, e->enable); +#endif return -1; } } @@ -536,8 +563,9 @@ const char *cname, *vname; char desc[80]; u_int8_t model, step; - unsigned i, j, k, old; + unsigned i, j, k, bit, old; u_int32_t id; + int reg; snd_mtxlock(codec->lock); codec->count = AC97_INIT(codec->methods, codec->devinfo); @@ -552,6 +580,16 @@ ac97_wrcd(codec, AC97_REG_POWER, (codec->flags & AC97_F_EAPD_INV)? 0x8000 : 0x0000); i = ac97_rdcd(codec, AC97_REG_RESET); + j = ac97_rdcd(codec, AC97_REG_RESET); + /* + * Let see if this codec can return consistent value. + * If not, turn on aggressive read workaround + * (STAC9704 comes in mind). + */ + if (i != j) { + codec->flags |= AC97_F_RDCD_BUG; + i = ac97_rdcd(codec, AC97_REG_RESET); + } codec->caps = i & 0x03ff; codec->se = (i & 0x7c00) >> 10; @@ -610,36 +648,55 @@ for (i = 0; i < 32; i++) { k = codec->noext? codec->mix[i].enable : 1; - if (k && (codec->mix[i].reg > 0)) { - j = old = ac97_rdcd(codec, codec->mix[i].reg); - if (!(j & 0x8000)) { - ac97_wrcd(codec, codec->mix[i].reg, j | 0x8000); - j = ac97_rdcd(codec, codec->mix[i].reg); - } + reg = codec->mix[i].reg; + if (reg < 0) + reg = -reg; + if (k && reg) { + j = old = ac97_rdcd(codec, reg); + /* + * Test for mute bit (except for AC97_MIX_TONE, + * where we simply assume it as available). + */ + if (codec->mix[i].mute) { + ac97_wrcd(codec, reg, j | 0x8000); + j = ac97_rdcd(codec, reg); + } else + j |= 0x8000; if ((j & 0x8000)) { - j = ((1 << 6) - 1) << codec->mix[i].ofs; - if (codec->mix[i].mute) - j |= 0x8000; - ac97_wrcd(codec, codec->mix[i].reg, j); - j = ac97_rdcd(codec, codec->mix[i].reg) & j; - j >>= codec->mix[i].ofs; - if (codec->mix[i].reg == AC97_MIX_TONE && - ((j & 0x0001) == 0x0000)) - j >>= 1; - for (k = 0; j != 0; k++) - j >>= 1; - for (j = 0; k != 0; j++) + /* + * Test whether the control width should be + * 4, 5 or 6 bit. For 5bit register, we should + * test it whether it's really 5 or 6bit. Leave + * 4bit register alone, because sometimes an + * attempt to write past 4th bit may cause + * incorrect result especially for AC97_MIX_BEEP + * (ac97 2.3). + */ + bit = codec->mix[i].bits; + if (bit == 5) + bit++; + j = ((1 << bit) - 1) << codec->mix[i].ofs; + ac97_wrcd(codec, reg, + j | (codec->mix[i].mute ? 0x8000 : 0)); + k = ac97_rdcd(codec, reg) & j; + k >>= codec->mix[i].ofs; + if (reg == AC97_MIX_TONE && + ((k & 0x0001) == 0x0000)) k >>= 1; + for (j = 0; k >> j; j++) + ; if (j != 0) { - codec->mix[i].enable = 1; #if 0 - codec->mix[i].bits = j; + device_printf(codec->dev, "%2d: [ac97_rdcd() = %d] [Testbit = %d] %d -> %d\n", + i, k, bit, codec->mix[i].bits, j); #endif + codec->mix[i].enable = 1; + codec->mix[i].bits = j; } else codec->mix[i].enable = 0; } else codec->mix[i].enable = 0; - ac97_wrcd(codec, codec->mix[i].reg, old); + ac97_wrcd(codec, reg, old); } #if 0 printf("mixch %d, en=%d, b=%d\n", i, codec->mix[i].enable, codec->mix[i].bits); @@ -650,6 +707,8 @@ ac97_hw_desc(codec->id, vname, cname, desc)); if (bootverbose) { + if (codec->flags & AC97_F_RDCD_BUG) + device_printf(codec->dev, "Buggy AC97 Codec: aggressive ac97_rdcd() workaround enabled\n"); device_printf(codec->dev, "Codec features "); for (i = j = 0; i < 10; i++) if (codec->caps & (1 << i)) --- sys/dev/sound/pcm/ac97.h.orig Thu Jan 6 09:43:20 2005 +++ sys/dev/sound/pcm/ac97.h Mon Aug 15 15:41:42 2005 @@ -81,6 +81,7 @@ #define AC97_REG_ID2 0x7e #define AC97_F_EAPD_INV 0x00000001 +#define AC97_F_RDCD_BUG 0x00000002 #define AC97_DECLARE(name) static DEFINE_CLASS(name, name ## _methods, sizeof(struct kobj)) #define AC97_CREATE(dev, devinfo, cls) ac97_create(dev, devinfo, &cls ## _class) -- DIFF END -- -- Ariff Abdullah MyBSD http://www.MyBSD.org.my (IPv6/IPv4) http://staff.MyBSD.org.my (IPv6/IPv4) http://tomoyo.MyBSD.org.my (IPv6/IPv4)