diff mbox series

Quirks for MacroSilicon MS2100/MS2106

Message ID 795d8e1a-8fc7-2302-613e-ff1743de5c16@pelago.org.uk (mailing list archive)
State New, archived
Headers show
Series Quirks for MacroSilicon MS2100/MS2106 | expand

Commit Message

John Veness June 22, 2022, 8:23 p.m. UTC
Hello,

This is my first ever kernel work, so I hope I am doing everything
correctly.

I have a USB analogue AV capture dongle with a MacroSilicon MS2100E chip
inside. PID 0x534d, VID 0x0021 (534d:0021). Apparently the MS2106 uses
the same PID/VID, so hopefully acts the same, but I don't have one to check.

For audio, the MS2100E has the same problems as the MS2109 USB HDMI
capture dongle, namely claiming 96kHz mono sound when it's actually
48kHz stereo, and with left/right channel problems.

Luckily, the fix is the exact same as that for the MS2109, as first
implemented by Marcan (Hector Martin) in July 2020. Below is a patch
which is basically a copy and paste of his, with a different VID.

I have tested this patch in 5.19.0 RC2. I haven't recruited any other
testers.

Even with this patch, there is a remaining problem, which is not present
in the MS2109. The sound sample values range from 0x0000 to 0x7fff, with
silence around 0x4000, i.e. 15-bit-ish audio. This actually sounds OK to
the ear (although half as loud as it should be), but looks odd when
looking at the waveform, and makes volume meters always think the sound
is very loud.

To convert to s16le, I can bitshift one bit left, and subtract 32768.
I'm told that this isn't something that can or should be done in the
kernel, but should be in userspace. Any more advice on how to fix this
remaining quirk would be very welcome.

Nonetheless, as I say, with the following kernel patch the captured
audio certainly sounds right, so is a big improvement and makes these
dongles usable:



Thanks for reading and considering this patch.

John Veness

Comments

John Veness June 24, 2022, 2:11 p.m. UTC | #1
On 23/06/2022 06:58, Takashi Iwai wrote:
>> Nonetheless, as I say, with the following kernel patch the captured
>> audio certainly sounds right, so is a big improvement and makes these
>> dongles usable:
> 
> So as a first step, we can merge the patch as is.  The rest needs more
> consideration.
> 
> In anyway, for merging the patch, it has to be submitted properly in a
> right format, especially including your Signed-off-by tag.  Please
> refer to Documentation/process/submitting-patches.rst for details.
> 
> thanks,
> 
> Takashi

Thank you, or the others, for the encouraging comments. I have just 
submitted the patch in the proper format... I hope!

John Veness
Takashi Sakamoto June 25, 2022, 3:08 a.m. UTC | #2
On Thu, Jun 23, 2022 at 10:51:18AM +0200, Jaroslav Kysela wrote:
> On 23. 06. 22 10:24, Takashi Sakamoto wrote:
> > Hi,
> > 
> > On Thu, Jun 23, 2022, at 16:18, Jaroslav Kysela wrote:
> > > On 23. 06. 22 7:58, Takashi Iwai wrote:
> > > 
> > > > > Even with this patch, there is a remaining problem, which is not present
> > > > > in the MS2109. The sound sample values range from 0x0000 to 0x7fff, with
> > > > > silence around 0x4000, i.e. 15-bit-ish audio. This actually sounds OK to
> > > > > the ear (although half as loud as it should be), but looks odd when
> > > > > looking at the waveform, and makes volume meters always think the sound
> > > > > is very loud.
> > > > > 
> > > > > To convert to s16le, I can bitshift one bit left, and subtract 32768.
> > > > > I'm told that this isn't something that can or should be done in the
> > > > > kernel, but should be in userspace. Any more advice on how to fix this
> > > > > remaining quirk would be very welcome.
> > > > 
> > > > Ouch, this is painful.  We haven't had any devices that require a
> > > > 15 bit unsigned format, and maybe we don't want to add it to the
> > > > common standard format just for one funky device, either.  Such data
> > > > processing could be done in alsa-lib, but for the proper interaction
> > > > with the user-space, the kernel should provide some information so
> > > > that user-space can process the data accordingly.  However, we have no
> > > > proper way defined for it generically, so far.
> > > > 
> > > > Maybe an easy way would be to create an alsa-lib external plugin, and
> > > > apply it per device.  Jaroslav, could it be done via UCM?
> > > 
> > > I agree that we may start with a special plugin for this format. The UCM can
> > > use any alsa-lib configuration now. So PA/PW should work with this very
> > > specific hardware when properly configured.
> > > 
> > > Note that we have SNDRV_PCM_FORMAT_SPECIAL for such cases. It will imply that
> > > the applications will fail when the special conversion plugin is not used. The
> > > minor issue may be with the silence routines (which is already with the
> > > improper format).
> > 
> > I think the combination of format/subformat is available in the case.
> > 
> > Actually the odd frame format is within 16 bit slot, so SNDRV_PCM_FORMAT_U16
> > or so is available. Then we can define new subformat to notify userspace; e.g.
> > SNDRV_PCM_SUBFORMAT_MODEL_SPECIFIC.
> > 
> > The cons is that we need to add new subformat, thus update asound.h. On the
> > other hand, the pros is that existent userspace stuffs still handle the odd format
> > and user can hear playback sound at least even if the loudness is not expected.
> 
> It's a question if the cutted half-wave samples should be sent to D/A in
> this case. Also, if the high-bit (U16) is set, the resulting sample value
> for D/A is completely incorrect. It's not only about loudness.

Indeed. I overlooked in the report... Even if receiving 16 bit sample from
userspace, the device seems not to generate better sound. The conversion is
required for listening anyway.

> My opinion is to not use the U16 sample format for this and handle this
> hw as special one until the format is more common (which I do not expect)
> to extend the format list.

If it's left-justified or right-padding case, msbits parameter shall be
available, but the case is not. If avoiding new entry for format
definitions (the rest is 12 entries in kernel API), it seems to be
inevitable to use SPECIAL format since hardware parameter doesn't deliver
explicit information about effective bits (=width) inner sample slot
(=physical width) for left-padding case.


Regards

Takashi Sakamoto
John Veness June 25, 2022, 10:17 a.m. UTC | #3
Thanks for all the discussions so far. A lot of it is over my head, but 
I appreciate it!

> Even with this patch, there is a remaining problem, which is not present > in the MS2109. The sound sample values range from 0x0000 to 0x7fff, 
with> silence around 0x4000, i.e. 15-bit-ish audio. This actually sounds 
OK to> the ear (although half as loud as it should be), but looks odd 
when> looking at the waveform, and makes volume meters always think the 
sound> is very loud.
On further inspection, the last hex digit of the samples are always 0 or 
8. So the least significant three bits are always zero, and technically 
the values range from 0x0000 to 0x7ff8, not 0x7fff as I said before.

> To convert to s16le, I can bitshift one bit left, and subtract 32768.
> I'm told that this isn't something that can or should be done in the
> kernel, but should be in userspace. Any more advice on how to fix this
> remaining quirk would be very welcome.

I don't know if this makes things more complicated. It is still the case 
that the conversion described above to s16le makes the waveforms look 
correct (and the volume correct), even if the lowest four bits are zero.

John Veness
diff mbox series

Patch

diff --git a/sound/usb/quirks-table.h b/sound/usb/quirks-table.h
index 4f56e1784932..853da162fd18 100644
--- a/sound/usb/quirks-table.h
+++ b/sound/usb/quirks-table.h
@@ -3802,6 +3802,54 @@  YAMAHA_DEVICE(0x7010, "UB99"),
         }
  },

+/*
+ * MacroSilicon MS2100/MS2106 based AV capture cards
+ *
+ * These claim 96kHz 1ch in the descriptors, but are actually 48kHz 2ch.
+ * They also need QUIRK_FLAG_ALIGN_TRANSFER, which makes one wonder if
+ * they pretend to be 96kHz mono as a workaround for stereo being broken
+ * by that...
+ *
+ * They also have an issue with initial stream alignment that causes the
+ * channels to be swapped and out of phase, which is dealt with in quirks.c.
+ */
+{
+       USB_AUDIO_DEVICE(0x534d, 0x0021),
+       .driver_info = (unsigned long) &(const struct snd_usb_audio_quirk) {
+               .vendor_name = "MacroSilicon",
+               .product_name = "MS210x",
+               .ifnum = QUIRK_ANY_INTERFACE,
+               .type = QUIRK_COMPOSITE,
+               .data = &(const struct snd_usb_audio_quirk[]) {
+                       {
+                               .ifnum = 2,
+                               .type = QUIRK_AUDIO_STANDARD_MIXER,
+                       },
+                       {
+                               .ifnum = 3,
+                               .type = QUIRK_AUDIO_FIXED_ENDPOINT,
+                               .data = &(const struct audioformat) {
+                                       .formats = SNDRV_PCM_FMTBIT_S16_LE,
+                                       .channels = 2,
+                                       .iface = 3,
+                                       .altsetting = 1,
+                                       .altset_idx = 1,
+                                       .attributes = 0,
+                                       .endpoint = 0x82,
+                                       .ep_attr = USB_ENDPOINT_XFER_ISOC |
+                                               USB_ENDPOINT_SYNC_ASYNC,
+                                       .rates = SNDRV_PCM_RATE_CONTINUOUS,
+                                       .rate_min = 48000,
+                                       .rate_max = 48000,
+                               }
+                       },
+                       {
+                               .ifnum = -1
+                       }
+               }
+       }
+},
+
  /*
   * MacroSilicon MS2109 based HDMI capture cards
   *
diff --git a/sound/usb/quirks.c b/sound/usb/quirks.c
index e8468f9b007d..a72874bc0936 100644
--- a/sound/usb/quirks.c
+++ b/sound/usb/quirks.c
@@ -1478,6 +1478,7 @@  void snd_usb_set_format_quirk(struct snd_usb_substream *subs,
         case USB_ID(0x041e, 0x3f19): /* E-Mu 0204 USB */
                 set_format_emu_quirk(subs, fmt);
                 break;
+       case USB_ID(0x534d, 0x0021): /* MacroSilicon MS2100/MS2106 */
         case USB_ID(0x534d, 0x2109): /* MacroSilicon MS2109 */
                 subs->stream_offset_adj = 2;
                 break;
@@ -1904,6 +1905,8 @@  static const struct usb_audio_quirk_flags_table quirk_flags_table[] = {
                    QUIRK_FLAG_IGNORE_CTL_ERROR),
         DEVICE_FLG(0x413c, 0xa506, /* Dell AE515 sound bar */
                    QUIRK_FLAG_GET_SAMPLE_RATE),
+       DEVICE_FLG(0x534d, 0x0021, /* MacroSilicon MS2100/MS2106 */
+                  QUIRK_FLAG_ALIGN_TRANSFER),
         DEVICE_FLG(0x534d, 0x2109, /* MacroSilicon MS2109 */
                    QUIRK_FLAG_ALIGN_TRANSFER),
         DEVICE_FLG(0x1224, 0x2a25, /* Jieli Technology USB PHY 2.0 */