[alsa-devel] USB Audio Interface / Denon MC7000 and MC8000 controller

[Alexander Tsoy]

В Пт, 07/02/2020 в 09:15 +0100, Takashi Iwai пишет:
> On Thu, 06 Feb 2020 23:09:33 +0100,
> Alexander Tsoy wrote:
> > В Чт, 06/02/2020 в 11:06 +0100, Tobias пишет:
> > > Thank you so much Alexander!
> > > I used latest Kernel and patched as you suggested. The Device is
> > > working 
> > > now giving sound on all 4 channels, even though dmesg still shows
> > > the 
> > > error message as you can see here:
> > > 
> > > uname -a:
> > > Linux tobias-V130 5.5.2 #1 SMP Thu Feb 6 09:41:57 CET 2020 x86_64
> > > x86_64 
> > > x86_64 GNU/Linux
> > > 
> > > dmesg:
> > > [   62.918777] usb 1-1.3: new high-speed USB device number 6
> > > using
> > > xhci_hcd
> > > [   62.939293] usb 1-1.3: New USB device found, idVendor=15e4, 
> > > idProduct=8004, bcdDevice=11.10
> > > [   62.939295] usb 1-1.3: New USB device strings: Mfr=1,
> > > Product=2, 
> > > SerialNumber=3
> > > [   62.939297] usb 1-1.3: Product: DENON DJ MC7000
> > > [   62.939298] usb 1-1.3: Manufacturer: DENON DJ
> > > [   62.939299] usb 1-1.3: SerialNumber: 201603
> > > [   62.942232] usb 1-1.3: clock source 65 is not valid, cannot
> > > use
> > > [   62.943998] usb 1-1.3: clock source 65 is not valid, cannot
> > > use
> > > [   63.013306] usb 1-1.3: clock source 65 is not valid, cannot
> > > use
> > > [   63.028912] usb 1-1.3: clock source 65 is not valid, cannot
> > > use
> > > [   63.029675] usb 1-1.3: clock source 65 is not valid, cannot
> > > use
> > > [   63.037813] usb 1-1.3: clock source 65 is not valid, cannot
> > > use
> > > [   63.063865] usb 1-1.3: clock source 65 is not valid, cannot
> > > use
> > 
> > Yes, this is expected.
> > 
> > > I checked in file /sound/usb/clock.c that within functions
> > > 
> > > static int __uac_clock_find_source
> > > static int __uac3_clock_find_source
> > > 
> > > there is another check that possibly gives the warning.
> > > 
> > > Maybe the warning "cannot use" should not be displayed when a
> > > Denon 
> > > Audio device is attached as it is misleading.
> > 
> > Please try the patch below. I've dropped UAC3 support and changed
> > __uac_clock_find_source() and __uac3_clock_find_source() to print
> > errors only in debug mode, as we make the final decision about
> > clock
> > validity in set_sample_rate_v2v3().
> > 
> > 
> > Dear Takashi, what do you think about this approach. Is it
> > acceptable?
> 
> Yes, the approach looks good to me.
> Just a few comments:
> 
> > diff --git a/sound/usb/clock.c b/sound/usb/clock.c
> > index 018b1ecb5404..e978b46efc85 100644
> > --- a/sound/usb/clock.c
> > +++ b/sound/usb/clock.c
> > @@ -197,6 +197,32 @@ static bool uac_clock_source_is_valid(struct
> > snd_usb_audio *chip,
> >  	return data ? true :  false;
> >  }
> >  
> > +/*
> > + * Assume the clock is valid if clock source supports only one
> > single sample
> > + * rate, its type is not external and a terminal is connected
> > directly to it
> > + * (there is no clock selector). This is needed for some Denon DJ
> > controllers,
> > + * that always reports that clock is invalid.
> > + */
> > +static bool uac_clock_source_is_valid_quirk(struct snd_usb_audio
> > *chip,
> > +					    struct audioformat *fmt,
> > +					    int clock)
> > +{
> > +	if (fmt->protocol == UAC_VERSION_2) {
> > +		struct uac_clock_source_descriptor *cs_desc =
> > +			snd_usb_find_clock_source(chip->ctrl_intf,
> > clock);
> > +
> > +		if (!cs_desc)
> > +			return false;
> > +
> > +		return (fmt->nr_rates == 1 &&
> > +			(fmt->clock & 0xff) == cs_desc->bClockID &&
> > +			(cs_desc->bmAttributes & 0x3) !=
> > +				UAC_CLOCK_SOURCE_TYPE_EXT);
> > +	}
> > +
> > +	return false;
> 
> IMO it's safer to call from the specific failure path, i.e. 
> 
>  static bool uac_clock_source_is_valid(....)
>  {
> 	....
> 	err = snd_usb_ctl_msg(dev, usb_rcvctrlpipe(dev, 0),
> UAC2_CS_CUR,
> 			      USB_TYPE_CLASS | USB_RECIP_INTERFACE |
> USB_DIR_IN,
> 			      UAC2_CS_CONTROL_CLOCK_VALID << 8,
> 			      snd_usb_ctrl_intf(chip) | (source_id <<
> 8),
> 			      &data, sizeof(data));
> 
> 	if (err < 0) {
> 
> 		if (uac_clock_source_is_valid_quirk(....))
> 			return true;
> 
> 		dev_warn(&dev->dev,
> 			 "%s(): cannot get clock validity for id %d\n",
> 			   __func__, source_id);
> 		return false;
> 	}
> 
> Then you can pass cs_desc there, too.

Thank you for the feedback!

I just realized that we haven't checked where
uac_clock_source_is_valid() is actually failing. There are two
possibilities:
- the Clock Validity Control is actually not present (err < 0), so
there is an error in descriptors;
- the Clock Validity Control request always returns FALSE (data =
false).

> 
> 
> > +}
> > +
> >  static int __uac_clock_find_source(struct snd_usb_audio *chip, int
> > entity_id,
> >  				   unsigned long *visited, bool
> > validate)
> >  {
> > @@ -219,7 +245,7 @@ static int __uac_clock_find_source(struct
> > snd_usb_audio *chip, int entity_id,
> >  		entity_id = source->bClockID;
> >  		if (validate && !uac_clock_source_is_valid(chip,
> > UAC_VERSION_2,
> >  								entity_
> > id)) {
> > -			usb_audio_err(chip,
> > +			usb_audio_dbg(chip,
> >  				"clock source %d is not valid, cannot
> > use\n",
> >  				entity_id);
> >  			return -ENXIO;
> 
> Hm, it's not good to hide the error message always.  This is a common
> error on many devices and suppressing it would look cleaner but also
> hide what's the reason.  Maybe we can add nowarn bool flag for
> certain
> code paths?

We can print the error only once at the end of set_sample_rate_v2v3(),
where uac_clock_source_is_valid() is called for the last time:

@@ -578,6 +606,9 @@ static int set_sample_rate_v2v3(struct snd_usb_audio *chip, int iface,
 validation:
    /* validate clock after rate change */
    if (!uac_clock_source_is_valid(chip, fmt->protocol, clock))
+       usb_audio_err(chip,
+                 "clock source %d is not valid, cannot use\n",
+                 clock);
        return -ENXIO;
    return 0;
 }

> 
> 
> thanks,
> 
> Takashi