Re: [PATCH v6 2/4] HID: playstation: add microphone mute support for DualSense.

[Benjamin Tissoires <benjamin.tissoires@redhat.com>]

On Tue, Feb 16, 2021 at 5:41 PM Marek Behun <marek.behun@nic.cz> wrote:
>
> On Tue, 16 Feb 2021 00:33:24 -0800
> Roderick Colenbrander <roderick@gaikai.com> wrote:
>
> > On Mon, Feb 15, 2021 at 10:17 AM Marek Behun <marek.behun@nic.cz> wrote:
> > >
> > > On Mon, 15 Feb 2021 10:07:29 -0800
> > > Roderick Colenbrander <roderick@gaikai.com> wrote:
> > >
> > > > On Mon, Feb 15, 2021 at 6:40 AM Marek Behun <marek.behun@nic.cz> wrote:
> > > > >
> > > > > On Sun, 14 Feb 2021 16:45:47 -0800
> > > > > Roderick Colenbrander <roderick@gaikai.com> wrote:
> > > > >
> > > > > > From: Roderick Colenbrander <roderick.colenbrander@sony.com>
> > > > > >
> > > > > > The DualSense controller has a built-in microphone exposed as an
> > > > > > audio device over USB (or HID using Bluetooth). A dedicated
> > > > > > button on the controller handles mute, but software has to configure
> > > > > > the device to mute the audio stream.
> > > > > >
> > > > > > This patch captures the mute button and schedules an output report
> > > > > > to mute/unmute the audio stream as well as toggle the mute LED.
> > > > > >
> > > > > > Signed-off-by: Roderick Colenbrander <roderick.colenbrander@sony.com>
> > > > >
> > > > > Is the microphone supported via Linux? I.e. is there an audio driver
> > > > > for it?
> > > >
> > > > Yes and no. The microphone is supported using USB, not yet using
> > > > Bluetooth (uses a custom protocol). Actually there are various other
> > > > audio features in the DualSense (headphone jack, speaker, volume
> > > > controls,..) and they all work using custom protocols. We were
> > > > planning to defer this work through future patches as the features are
> > > > very complicated and need a deep analysis on how to realize them. For
> > > > example audio controls work through HID, but for USB the audio driver
> > > > is a generic hda audio device I think. Bluetooth is a custom protocol
> > > > and will be yet a different audio driver somewhere.
> > > >
> > > > > If it is, look at the audio-micmute LED trigger.
> > > > >
> > > >
> > > > I'm not sure if the expected behavior for the DualSense is similar to
> > > > the standard audio mute use cases. My understanding of these triggers
> > > > (please correct me if I'm wrong) is for e.g. an audio driver or user
> > > > space to send a signal to anything registering for a particular
> > > > trigger. In this case a global micmute. Is that, right?
> > > >
> > > > In our case for PlayStation games, there are often multiple
> > > > controllers connected and each user has their own microphone in their
> > > > controller. All can function at the same time (different from a
> > > > standard PC use case). That's why I'm wondering if this makes sense.I
> > > > know we are on Linux, but for Sony we want to properly support such
> > > > use cases.
> > >
> > > If there aren't audio drivers yet for this, simply have this driver
> > > also register a private LED trigger (with name "joystick-audiomute"
> > > or something similar), and when registering the LED, set the
> > > trigger_type member. Look at trigger_type in include/linux/leds.h, and
> > > in LED Documentation.
> >
> > Sorry for some more questions. I have been trying to understand
> > triggers all night. The concept is just so strange and foreign to me.
> > I understand it is in the end just a string and one use case is
> > in-kernel IPC and you can configure them from user space as well, but
> > I just don't get it. I understand you can use a trigger to in the end
> > program your LED in a automatic manner. I just don't understand how
> > the concepts fit together and how to implement it (maybe I will update
> > the docs later on... they are a bit sparse for if you don't know this
> > area).
> >
> > Regarding registering a private trigger. I see include/linux/leds.h
> > have a comment about trigger_type and how it should be set for private
> > triggers on led_classdev. I haven't been able to find any example
> > usages of this within the kernel. It doesn't seem to be used in the
> > kernel, maybe it is just around for future use? I also seem to need to
> > implement my own activate/deactive callbacks for the trigger. These I
> > would use to program the LED brightness I guess. Though I see various
> > trigger drivers (drivers/leds/triggers), but not all of them have
> > activate/deactivate callbacks. Mostly simple drivers, but not sure why
> > they don't need them. What else is the point of a trigger?
> >
> > > When this trigger is enabled for your LED, have your code switch LED
> > > state like it does now. When there is no trigger enabled, the userspace
> > > will be able to set brightness of this LED via sysfs.
> >
> > Right now I manage the button mute state directly from the input
> > handler (dualsense_parse_report) when the button is pressed and then
> > schedule an output report to toggle the LED and program the DualSense
> > to mute its audio (the PlayStation works very similar). I would need
> > to use led_trigger_event then here?
> >
> > If I then understand it right, I need to modify my "brightness_set"
> > handler and check if there is a trigger (based on
> > led_classdev->activated??). If there is none, then userspace can
> > change the LED state. Internally when I change the LED state, I will
> > also program the hardware to mute as well. (they are tied together)
> >
> > I am tempted to wait with the trigger code as I really don't understand it.
>
> Simple triggers are just normal triggers but with some simplifying code
> to avoid code repetition. Ignore them for now.
>
> When a trigger is set to a LED via sysfs, the trigger .activate()
> method is called and the led_cdev.trigger is set to point to that
> trigger.
>
> It is then up to the code inside the trigger's .activate() method to
> initialize mechanisms that will control the LED.
>
> For netdev trigger a delayed_work is scheduled periodically, and in each
> execution of that work's callback the netdevice's stats are compared to
> the last ones. If the new stats are greater, the trigger code blinks the
> LED.
>
> So in your case it is pretty simple to implement, because you already
> have the necessary code to manipulate the LED brightness automatically
> according to whether button was pressed. You are setting
>   ds->update_mic_mute = true;
> in dualsense_parse_report() and then manipulate the LED in
> dualsense_output_worker().
>
> Just add another boolean member into struct dualsense:
>   bool control_mute_led;
> and change the code in dualsense_output_worker() to only change the
> mute_led brightness is this new member is true.
>
> Add this code to your driver:
>
>   static struct led_hw_trigger_type ps_micmute_trigger_type;
>
> When registering the LED in ps_led_register(), also set
>   led->trigger_type = &ps_micmute_trigger_type;
>
> Add this functions:
>   static int ps_micmute_trig_activate(struct led_classdev *led_cdev)
>   {
>     struct dualsense *ds = container_of(...);
>
>     /* make the worker control mute LED according to mute button */
>     ds->control_mute_led = true;
>
>     /* make sure the mute LED shows the current mute button state */
>     ds->update_mic_mute = true;
>     schedule_work(&ds->output_worker);
>
>     return 0;
>   }
>
>   static void ps_micmute_trig_deactivate(struct led_classdev *led_cdev)
>   {
>     struct dualsense *ds = container_of(...);
>
>     ds->control_mute_led = false;
>   }
>
>   static struct led_trigger ps_micmute_trigger = {
>     .name = "playstation-micmute",
>     .activate = ps_micmute_trig_activate,
>     .deactivate = ps_micmute_trig_deactivate,
>     .trigger_type = &ps_micmute_trigger_type,
>   };
>
> Add this code to ps_init():
>   int ret;
>
>   ret = led_trigger_register(&ps_micmute_trigger);
>   if (ret)
>     return ret;
>
> And to ps_exit():
>   led_trigger_unregister(&ps_micmute_trigger);
>
> All this will make sure that the driver will manipulate the mute
> LED state only when the playstation-micmute trigger is active on the
> LED.
>
> Moreover if you want this driver to be active on the LED by default,
> set this prior to registering the LED
>   led->default_trigger = "playstation-micmute";
>
> Finally add code to dualsense_mute_led_set_brightness() to make
> userspace/[other LED triggers] able to set mute LED brightness.
>
> The purpose of the .trigger_type member of struct led_classdev and
> struct led_trigger is that if this member is set for a trigger, this
> trigger will only be available for LEDs that have the same trigger_type.
>

Thanks Marek for the in-depth 101 on LED triggers :)

However, I am not sure we want to enable LED triggers for the micmute
on the controller itself. In the early discussions with Roderick, I
already suggested the use of the LED triggers, and the problem was
that they are shared system-wide. This is good for many use cases, but
in that particular case, the user expects the mic *of the controller*
to be muted, not everyone's controller's mics.This is a behavior
inherited from the Playstation 5 which would be hard to sell to owners
of the controllers.

So if read-only LEDs are not an option, how about we simply ditch the
LED for micmute in the current code, and have a simple callback
executed by the driver to light up or not the LED when the player
presses the key. Or just revert entirely this commit in the currently
staged series. We can then figure out a better way in the next future
to handle that part.

BTW, AFAICT, the only problem we have left (if we put that micmute
issue aside) is about the naming convention. If we fix the naming
shortly, would you have any concerns if we still push that code to
Linus in 5.12-rc0?

Cheers,
Benjamin