Bell confusion with master vs. slave devices in Xorg 1.7.6

[Peter Hutterer]

On Sun, Apr 18, 2010 at 11:48:49AM -0700, Jeremy Huddleston wrote:
> 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?  

that's quite likely. clients can connect before we get anything from HAL or
udev, so in the xinitrc case it's possible that the devices don't even exist
yet.

> Why not have the slave inherit the volume level of the master when it is
> attached?

Careful here - a client may change the bell on just one device and detach it
(or implicitly detach with a grab). In this case, the device shouldn't
inherit the bell volume from the master again - that's why it was set on the
device directly. 

Where the above approach would work is on the first EnableDevice() for this
device - this is guaranteed to be the first attachment and the one where we
may change the bell volume based on the MD.

Cheers,
  Peter

> 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