Bell confusion with master vs. slave devices in Xorg 1.7.6

[Jeremy Huddleston]

So, I've finally read through this and I'm still not sure what the right solution is, but here are my 2 cents.

The real problem is the growing divide between Core and XInput.

The VCK should not result in an audio beep as it is a virtual device.

ProcBell should iterate through all physical (slave) devices and make those beep.

We should be able to control the volume for each device (via Xinput).

Setting volume through Core (xset b 0) should result in setting the volume on master (even though it doesn't beep... see below) and all slaves.

The deep copy should still copy bell proc (IMO) since that is what I'd expect from something called "deep copy"...

Why doesn't making DoChangeKeyboardControl set the volume on all slaves solve the .xinitrc issue?  Because the slave isn't attached when xinitrc is run?  Why not have the slave inherit the volume level of the master when it is attached?

--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