Bell confusion with master vs. slave devices in Xorg 1.7.6

[Jeremy Huddleston]

Yeah... I was seeing this as well.  I brought it up last month:
http://lists.x.org/archives/xorg-devel/2010-March/006265.html

And ended up just letting core ring it (DDXRingBell):
http://cgit.freedesktop.org/~jeremyhu/xserver/commit/?id=eac7cdabecafb7c505795207182ab2578d672c06

I don't have a chance to read/respond to this right now, but there might be something useful in that thread.

--Jeremy

On Apr 16, 2010, at 22:27, Alan Coopersmith wrote:

> Several users have complained to me about their X server bells on
> recent builds of OpenSolaris - definitely with Xorg 1.7.x releases,
> I don't know if it affected Xorg 1.6.x before that as well.
> 
> The complaints I've gotten are two-fold: the bell volume is
> uncontrollable, and when the beep sounds, it plays twice
> (stuck at the loud volume).
> 
> It looks like we're seeing this because unlike the evdev driver,
> the kbd driver has a BellProc set to sound the keyboard device
> bell (if present - the Solaris kernel redirects to an audio
> device if not).
> 
> When the keyboard gets associated with the Virtual Core Keyboard,
> the DeepCopyDeviceClasses copies the BellProc pointer to the
> Virtual Core Keyboard.   When an XBell request comes in, the
> ProcBell function in dix/devices.c loops through all the devices,
> and calls the BellProc in the master and any associated slaves,
> resulting in the KbdBell() function from kbd_drv getting called
> twice, once off the Virtual Core Keyboard and once off the slave
> representing the physical keyboard.
> 
> If you try to silence this with 'xset b 0' though, it does not
> loop through all the devices, and only sets the volume on the
> master.   You can see the slave still have the default volume of
> 50 by running 'xinput get-feedbacks keyboard'.   When ProcBell
> loops through the devices though, it uses the volume from each
> device.
> 
> Fixing this portion seems a simple matter of changing ProcBell to use
> the master keyboard volume on all slaves of that device as well:
> 
> --- dix/devices.c~	Fri Apr 16 19:29:30 2010
> +++ dix/devices.c	Fri Apr 16 20:54:31 2010
> @@ -2037,7 +2037,7 @@
> 	    if (rc != Success)
> 		return rc;
>             XkbHandleBell(FALSE, FALSE, dev, newpercent,
> -                          &dev->kbdfeed->ctrl, 0, None, NULL, client);
> +                          &keybd->kbdfeed->ctrl, 0, None, NULL, client);
>         }
>     }
> 
> This would let you keep per-device bell settings, and have those used when
> you call XDeviceBell on the specific device instead of the master.  It could
> also allow you to have xset called in your .xinitrc and have its volume setting
> stick even if the physical keyboard hasn't been associated with the core device
> yet, but that is currently thwarted by the DeepCopy overriding the core devices
> keyboard feedback settings when the device is connected to the master.
> 
> Alternatively, DoChangeKeyboardControl could set the bell volumes on all the
> slaves, but that also doesn't solve the problem of initializing the settings
> from .xinitrc.
> 
> As for the double bell, the two options there seem to be either removing the
> copying of BellProc in the DeepCopy code, or changing ProcBell to call it on
> either the master or the slave, but not both.  Since the Solaris implementation
> of xf86OSRingBell also makes noise, via /dev/audio, I'm tempted to do both.
> By not copying BellProc, the virtual keyboard is left with it's BellProc set to
> CoreKeyboardBell, which unwraps down to xf86OSRingBell, so that would be used
> when no other bell device is present, but if any slaves have a bell, we could
> skip the master.
> 
> --- Xi/exevents.c~	Fri Apr 16 21:48:19 2010
> +++ Xi/exevents.c	Fri Apr 16 21:48:37 2010
> @@ -413,7 +413,6 @@
>                     return;
>                 }
>             }
> -            (*k)->BellProc = it->BellProc;
>             (*k)->CtrlProc = it->CtrlProc;
>             (*k)->ctrl     = it->ctrl;
>             if ((*k)->xkb_sli)
> 
> --- dix/devices.c~	2010-04-16 21:49:26.132804948 -0700
> +++ dix/devices.c	2010-04-16 22:19:16.742406867 -0700
> @@ -2015,6 +2015,7 @@ ProcBell(ClientPtr client)
>     int base = keybd->kbdfeed->ctrl.bell;
>     int newpercent;
>     int rc;
> +    int rung = 0;
>     REQUEST(xBellReq);
>     REQUEST_SIZE_MATCH(xBellReq);
> 
> @@ -2029,17 +2030,28 @@ ProcBell(ClientPtr client)
>     else
> 	newpercent = base - newpercent + stuff->percent;
> 
> +    /* first try to ring the bells on the slaves */
>     for (dev = inputInfo.devices; dev; dev = dev->next) {
> -        if ((dev == keybd || (!IsMaster(dev) && dev->u.master == keybd)) &&
> +        if ((!IsMaster(dev) && dev->u.master == keybd) &&
>             dev->kbdfeed && dev->kbdfeed->BellProc) {
> 
> 	    rc = XaceHook(XACE_DEVICE_ACCESS, client, dev, DixBellAccess);
> -	    if (rc != Success)
> -		return rc;
> -            XkbHandleBell(FALSE, FALSE, dev, newpercent,
> -                          &keybd->kbdfeed->ctrl, 0, None, NULL, client);
> +	    if (rc == Success) {
> +		XkbHandleBell(FALSE, FALSE, dev, newpercent,
> +			      &keybd->kbdfeed->ctrl, 0, None, NULL, client);
> +		rung++;
> +	    }
>         }
>     }
> +    /* if no slaves found with bells, ring the master's bell */
> +    if (!rung && keybd->kbdfeed && keybd->kbdfeed->BellProc) {
> +
> +	rc = XaceHook(XACE_DEVICE_ACCESS, client, keybd, DixBellAccess);
> +	if (rc != Success)
> +	    return rc;
> +	XkbHandleBell(FALSE, FALSE, keybd, newpercent,
> +		      &keybd->kbdfeed->ctrl, 0, None, NULL, client);
> +    }
> 
>     return Success;
> }
> 
> Are these the right approaches to take?  Is there a better way to solve these
> problems before the bells drive the users batty?
> 
> -- 
> 	-Alan Coopersmith-        alan.coopersmith at oracle.com
> 	 Oracle Solaris Platform Engineering: X Window System
> 
> _______________________________________________
> xorg-devel at lists.x.org: X.Org development
> Archives: http://lists.x.org/archives/xorg-devel
> Info: http://lists.x.org/mailman/listinfo/xorg-devel