From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 04FF8C4708D for ; Mon, 2 Jan 2023 17:28:58 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S234259AbjABR25 (ORCPT ); Mon, 2 Jan 2023 12:28:57 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:35306 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S232808AbjABR2m (ORCPT ); Mon, 2 Jan 2023 12:28:42 -0500 Received: from smtp-out1.suse.de (smtp-out1.suse.de [IPv6:2001:67c:2178:6::1c]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 6B30019F; Mon, 2 Jan 2023 09:28:37 -0800 (PST) Received: from imap2.suse-dmz.suse.de (imap2.suse-dmz.suse.de [192.168.254.74]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (P-521) server-digest SHA512) (No client certificate requested) by smtp-out1.suse.de (Postfix) with ESMTPS id C7BE6342A7; Mon, 2 Jan 2023 17:28:35 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_rsa; t=1672680515; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=c+p+ZHZvJLaVRQr5NTszckwP5nV6LjSTDHOD89k5qNI=; b=ANkNie0iEw+MUPgAvLqNXpLtkCUzRqRfOgJpC594NUXYaDxiY0ZToLCCvayH8+/acLOskm WjgRbB9mrM9lwKA6rIRi2W++cSewn6f0kr15VS9qKwvPKpl5O5EPwiARv+2ujwKx5eFeKN 0w6nMhR/x82e6RGCWuEETudE7Id91l4= DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_ed25519; t=1672680515; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=c+p+ZHZvJLaVRQr5NTszckwP5nV6LjSTDHOD89k5qNI=; b=tWgOFZcqs4GLxyLAh3jp6i0EgE7JPjCDyRfptR9lzceBDEYk76L6j/PcrKE6zBOBjdSpj5 1KDBnaRWSnWbIVCg== Received: from imap2.suse-dmz.suse.de (imap2.suse-dmz.suse.de [192.168.254.74]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (P-521) server-digest SHA512) (No client certificate requested) by imap2.suse-dmz.suse.de (Postfix) with ESMTPS id 6BE0513427; Mon, 2 Jan 2023 17:28:35 +0000 (UTC) Received: from dovecot-director2.suse.de ([192.168.254.65]) by imap2.suse-dmz.suse.de with ESMTPSA id 6vZjGUMUs2ORIQAAMHmgww (envelope-from ); Mon, 02 Jan 2023 17:28:35 +0000 Date: Mon, 02 Jan 2023 18:28:34 +0100 Message-ID: <87edscsv5p.wl-tiwai@suse.de> From: Takashi Iwai To: Wesley Cheng Cc: , , , , , , , , , , , , , , , , , , , Subject: Re: [RFC PATCH 09/14] sound: usb: Introduce QC USB SND offloading support In-Reply-To: <20221223233200.26089-10-quic_wcheng@quicinc.com> References: <20221223233200.26089-1-quic_wcheng@quicinc.com> <20221223233200.26089-10-quic_wcheng@quicinc.com> User-Agent: Wanderlust/2.15.9 (Almost Unreal) Emacs/27.2 Mule/6.0 MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") Content-Type: text/plain; charset=US-ASCII Precedence: bulk List-ID: X-Mailing-List: linux-arm-msm@vger.kernel.org On Sat, 24 Dec 2022 00:31:55 +0100, Wesley Cheng wrote: > > Several Qualcomm SoCs have a dedicated audio DSP, which has the ability to > support USB sound devices. This vendor driver will implement the required > handshaking with the DSP, in order to pass along required resources that > will be utilized by the DSP's USB SW. The communication channel used for > this handshaking will be using the QMI protocol. Required resources > include: > - Allocated secondary event ring address > - EP transfer ring address > - Interrupter number > > The above information will allow for the audio DSP to execute USB transfers > over the USB bus. It will also be able to support devices that have an > implicit feedback and sync endpoint as well. Offloading these data > transfers will allow the main/applications processor to enter lower CPU > power modes, and sustain a longer duration in those modes. > > Signed-off-by: Wesley Cheng Hmm, this must be the main part that works to bypass the normal USB packet handling in USB audio driver but hooks to the own offload one, but there is no description how to take over and manage. A missing "big picture" makes it difficult to understand and review. Also, since both drivers are asynchronous, we may need some proper locking. More on the code change: > +static int snd_interval_refine_set(struct snd_interval *i, unsigned int val) > +{ > + struct snd_interval t; > + > + t.empty = 0; > + t.min = t.max = val; > + t.openmin = t.openmax = 0; > + t.integer = 1; > + return snd_interval_refine(i, &t); > +} > + > +static int _snd_pcm_hw_param_set(struct snd_pcm_hw_params *params, > + snd_pcm_hw_param_t var, unsigned int val, > + int dir) > +{ > + int changed; > + > + if (hw_is_mask(var)) { > + struct snd_mask *m = hw_param_mask(params, var); > + > + if (val == 0 && dir < 0) { > + changed = -EINVAL; > + snd_mask_none(m); > + } else { > + if (dir > 0) > + val++; > + else if (dir < 0) > + val--; > + changed = snd_mask_refine_set( > + hw_param_mask(params, var), val); > + } > + } else if (hw_is_interval(var)) { > + struct snd_interval *i = hw_param_interval(params, var); > + > + if (val == 0 && dir < 0) { > + changed = -EINVAL; > + snd_interval_none(i); > + } else if (dir == 0) > + changed = snd_interval_refine_set(i, val); > + else { > + struct snd_interval t; > + > + t.openmin = 1; > + t.openmax = 1; > + t.empty = 0; > + t.integer = 0; > + if (dir < 0) { > + t.min = val - 1; > + t.max = val; > + } else { > + t.min = val; > + t.max = val+1; > + } > + changed = snd_interval_refine(i, &t); > + } > + } else > + return -EINVAL; > + if (changed) { > + params->cmask |= 1 << var; > + params->rmask |= 1 << var; > + } > + return changed; > +} Those are taken from sound/core/oss/pcm_oss.c? We may put to the common PCM helper instead of duplication. > +static void disable_audio_stream(struct snd_usb_substream *subs) > +{ > + struct snd_usb_audio *chip = subs->stream->chip; > + > + if (subs->data_endpoint || subs->sync_endpoint) { > + close_endpoints(chip, subs); > + > + mutex_lock(&chip->mutex); > + subs->cur_audiofmt = NULL; > + mutex_unlock(&chip->mutex); > + } > + > + snd_usb_autosuspend(chip); > +} > + > +static int enable_audio_stream(struct snd_usb_substream *subs, > + snd_pcm_format_t pcm_format, > + unsigned int channels, unsigned int cur_rate, > + int datainterval) > +{ > + struct snd_usb_audio *chip = subs->stream->chip; > + struct snd_pcm_hw_params params; > + const struct audioformat *fmt; > + int ret; > + > + _snd_pcm_hw_params_any(¶ms); > + _snd_pcm_hw_param_set(¶ms, SNDRV_PCM_HW_PARAM_FORMAT, > + pcm_format, 0); > + _snd_pcm_hw_param_set(¶ms, SNDRV_PCM_HW_PARAM_CHANNELS, > + channels, 0); > + _snd_pcm_hw_param_set(¶ms, SNDRV_PCM_HW_PARAM_RATE, > + cur_rate, 0); What about other parameters like period / buffer sizes? > +struct qmi_uaudio_stream_req_msg_v01 { > + u8 enable; > + u32 usb_token; > + u8 audio_format_valid; > + u32 audio_format; > + u8 number_of_ch_valid; > + u32 number_of_ch; > + u8 bit_rate_valid; > + u32 bit_rate; > + u8 xfer_buff_size_valid; > + u32 xfer_buff_size; > + u8 service_interval_valid; > + u32 service_interval; > +}; Are this and the other structs a part of DSP ABI? Or is it a definition only used in kernel? I'm asking because __packed attribute is required for most of ABI definitions with different field types. thanks, Takashi From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from alsa0.perex.cz (alsa0.perex.cz [77.48.224.243]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 8B082C4708E for ; Mon, 2 Jan 2023 17:29:37 +0000 (UTC) Received: from alsa1.perex.cz (alsa1.perex.cz [207.180.221.201]) (using TLSv1.2 with cipher ADH-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by alsa0.perex.cz (Postfix) with ESMTPS id B357FA3E2; Mon, 2 Jan 2023 18:28:44 +0100 (CET) DKIM-Filter: OpenDKIM Filter v2.11.0 alsa0.perex.cz B357FA3E2 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=alsa-project.org; s=default; t=1672680574; bh=MI1x6fEM2sK/1axp690XjurD3OaPPC0EQLi7SICuzek=; h=Date:From:To:Subject:In-Reply-To:References:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: Cc:From; b=RoqZCHZWC3zIuHhX9iHqz020xNnKHXKuNs+3AcsBGYV48C6Q4uRRcJ7MGod5WBMy7 CJB3zC3i+w+9mQ0DO3vppIU2+/GkiU3OY167ed7mCCAm3C2FF3eaDrfkHpuB5FraVi saFA2+403DP6INUojRtadW8b3JdNCAIAUllOkzYM= Received: from alsa1.perex.cz (localhost.localdomain [127.0.0.1]) by alsa1.perex.cz (Postfix) with ESMTP id 5F5D4F80238; Mon, 2 Jan 2023 18:28:44 +0100 (CET) Received: by alsa1.perex.cz (Postfix, from userid 50401) id 59EB9F800C0; Mon, 2 Jan 2023 18:28:42 +0100 (CET) Received: from smtp-out1.suse.de (smtp-out1.suse.de [195.135.220.28]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) by alsa1.perex.cz (Postfix) with ESMTPS id 5A296F800C0 for ; Mon, 2 Jan 2023 18:28:36 +0100 (CET) DKIM-Filter: OpenDKIM Filter v2.11.0 alsa1.perex.cz 5A296F800C0 Authentication-Results: alsa1.perex.cz; dkim=pass (1024-bit key, unprotected) header.d=suse.de header.i=@suse.de header.a=rsa-sha256 header.s=susede2_rsa header.b=ANkNie0i; dkim=pass header.d=suse.de header.i=@suse.de header.a=ed25519-sha256 header.s=susede2_ed25519 header.b=tWgOFZcq Received: from imap2.suse-dmz.suse.de (imap2.suse-dmz.suse.de [192.168.254.74]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (P-521) server-digest SHA512) (No client certificate requested) by smtp-out1.suse.de (Postfix) with ESMTPS id C7BE6342A7; Mon, 2 Jan 2023 17:28:35 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_rsa; t=1672680515; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=c+p+ZHZvJLaVRQr5NTszckwP5nV6LjSTDHOD89k5qNI=; b=ANkNie0iEw+MUPgAvLqNXpLtkCUzRqRfOgJpC594NUXYaDxiY0ZToLCCvayH8+/acLOskm WjgRbB9mrM9lwKA6rIRi2W++cSewn6f0kr15VS9qKwvPKpl5O5EPwiARv+2ujwKx5eFeKN 0w6nMhR/x82e6RGCWuEETudE7Id91l4= DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_ed25519; t=1672680515; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=c+p+ZHZvJLaVRQr5NTszckwP5nV6LjSTDHOD89k5qNI=; b=tWgOFZcqs4GLxyLAh3jp6i0EgE7JPjCDyRfptR9lzceBDEYk76L6j/PcrKE6zBOBjdSpj5 1KDBnaRWSnWbIVCg== Received: from imap2.suse-dmz.suse.de (imap2.suse-dmz.suse.de [192.168.254.74]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (P-521) server-digest SHA512) (No client certificate requested) by imap2.suse-dmz.suse.de (Postfix) with ESMTPS id 6BE0513427; Mon, 2 Jan 2023 17:28:35 +0000 (UTC) Received: from dovecot-director2.suse.de ([192.168.254.65]) by imap2.suse-dmz.suse.de with ESMTPSA id 6vZjGUMUs2ORIQAAMHmgww (envelope-from ); Mon, 02 Jan 2023 17:28:35 +0000 Date: Mon, 02 Jan 2023 18:28:34 +0100 Message-ID: <87edscsv5p.wl-tiwai@suse.de> From: Takashi Iwai To: Wesley Cheng Subject: Re: [RFC PATCH 09/14] sound: usb: Introduce QC USB SND offloading support In-Reply-To: <20221223233200.26089-10-quic_wcheng@quicinc.com> References: <20221223233200.26089-1-quic_wcheng@quicinc.com> <20221223233200.26089-10-quic_wcheng@quicinc.com> User-Agent: Wanderlust/2.15.9 (Almost Unreal) Emacs/27.2 Mule/6.0 MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") Content-Type: text/plain; charset=US-ASCII X-BeenThere: alsa-devel@alsa-project.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: "Alsa-devel mailing list for ALSA developers - http://www.alsa-project.org" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: linux-arm-msm@vger.kernel.org, devicetree@vger.kernel.org, alsa-devel@alsa-project.org, linux-usb@vger.kernel.org, bgoswami@quicinc.com, mathias.nyman@intel.com, gregkh@linuxfoundation.org, andersson@kernel.org, tiwai@suse.com, lgirdwood@gmail.com, robh+dt@kernel.org, broonie@kernel.org, srinivas.kandagatla@linaro.org, agross@kernel.org, krzysztof.kozlowski+dt@linaro.org, Thinh.Nguyen@synopsys.com, quic_plai@quicinc.com, linux-kernel@vger.kernel.org, quic_jackp@quicinc.com Errors-To: alsa-devel-bounces@alsa-project.org Sender: "Alsa-devel" On Sat, 24 Dec 2022 00:31:55 +0100, Wesley Cheng wrote: > > Several Qualcomm SoCs have a dedicated audio DSP, which has the ability to > support USB sound devices. This vendor driver will implement the required > handshaking with the DSP, in order to pass along required resources that > will be utilized by the DSP's USB SW. The communication channel used for > this handshaking will be using the QMI protocol. Required resources > include: > - Allocated secondary event ring address > - EP transfer ring address > - Interrupter number > > The above information will allow for the audio DSP to execute USB transfers > over the USB bus. It will also be able to support devices that have an > implicit feedback and sync endpoint as well. Offloading these data > transfers will allow the main/applications processor to enter lower CPU > power modes, and sustain a longer duration in those modes. > > Signed-off-by: Wesley Cheng Hmm, this must be the main part that works to bypass the normal USB packet handling in USB audio driver but hooks to the own offload one, but there is no description how to take over and manage. A missing "big picture" makes it difficult to understand and review. Also, since both drivers are asynchronous, we may need some proper locking. More on the code change: > +static int snd_interval_refine_set(struct snd_interval *i, unsigned int val) > +{ > + struct snd_interval t; > + > + t.empty = 0; > + t.min = t.max = val; > + t.openmin = t.openmax = 0; > + t.integer = 1; > + return snd_interval_refine(i, &t); > +} > + > +static int _snd_pcm_hw_param_set(struct snd_pcm_hw_params *params, > + snd_pcm_hw_param_t var, unsigned int val, > + int dir) > +{ > + int changed; > + > + if (hw_is_mask(var)) { > + struct snd_mask *m = hw_param_mask(params, var); > + > + if (val == 0 && dir < 0) { > + changed = -EINVAL; > + snd_mask_none(m); > + } else { > + if (dir > 0) > + val++; > + else if (dir < 0) > + val--; > + changed = snd_mask_refine_set( > + hw_param_mask(params, var), val); > + } > + } else if (hw_is_interval(var)) { > + struct snd_interval *i = hw_param_interval(params, var); > + > + if (val == 0 && dir < 0) { > + changed = -EINVAL; > + snd_interval_none(i); > + } else if (dir == 0) > + changed = snd_interval_refine_set(i, val); > + else { > + struct snd_interval t; > + > + t.openmin = 1; > + t.openmax = 1; > + t.empty = 0; > + t.integer = 0; > + if (dir < 0) { > + t.min = val - 1; > + t.max = val; > + } else { > + t.min = val; > + t.max = val+1; > + } > + changed = snd_interval_refine(i, &t); > + } > + } else > + return -EINVAL; > + if (changed) { > + params->cmask |= 1 << var; > + params->rmask |= 1 << var; > + } > + return changed; > +} Those are taken from sound/core/oss/pcm_oss.c? We may put to the common PCM helper instead of duplication. > +static void disable_audio_stream(struct snd_usb_substream *subs) > +{ > + struct snd_usb_audio *chip = subs->stream->chip; > + > + if (subs->data_endpoint || subs->sync_endpoint) { > + close_endpoints(chip, subs); > + > + mutex_lock(&chip->mutex); > + subs->cur_audiofmt = NULL; > + mutex_unlock(&chip->mutex); > + } > + > + snd_usb_autosuspend(chip); > +} > + > +static int enable_audio_stream(struct snd_usb_substream *subs, > + snd_pcm_format_t pcm_format, > + unsigned int channels, unsigned int cur_rate, > + int datainterval) > +{ > + struct snd_usb_audio *chip = subs->stream->chip; > + struct snd_pcm_hw_params params; > + const struct audioformat *fmt; > + int ret; > + > + _snd_pcm_hw_params_any(¶ms); > + _snd_pcm_hw_param_set(¶ms, SNDRV_PCM_HW_PARAM_FORMAT, > + pcm_format, 0); > + _snd_pcm_hw_param_set(¶ms, SNDRV_PCM_HW_PARAM_CHANNELS, > + channels, 0); > + _snd_pcm_hw_param_set(¶ms, SNDRV_PCM_HW_PARAM_RATE, > + cur_rate, 0); What about other parameters like period / buffer sizes? > +struct qmi_uaudio_stream_req_msg_v01 { > + u8 enable; > + u32 usb_token; > + u8 audio_format_valid; > + u32 audio_format; > + u8 number_of_ch_valid; > + u32 number_of_ch; > + u8 bit_rate_valid; > + u32 bit_rate; > + u8 xfer_buff_size_valid; > + u32 xfer_buff_size; > + u8 service_interval_valid; > + u32 service_interval; > +}; Are this and the other structs a part of DSP ABI? Or is it a definition only used in kernel? I'm asking because __packed attribute is required for most of ABI definitions with different field types. thanks, Takashi