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 X-Spam-Level: X-Spam-Status: No, score=-8.8 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id DBF3BC433B4 for ; Mon, 12 Apr 2021 09:55:29 +0000 (UTC) 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 mail.kernel.org (Postfix) with ESMTPS id A06AF611AD for ; Mon, 12 Apr 2021 09:55:28 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org A06AF611AD Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=suse.de Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=alsa-devel-bounces@alsa-project.org Received: from alsa1.perex.cz (alsa1.perex.cz [207.180.221.201]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by alsa0.perex.cz (Postfix) with ESMTPS id 5728941; Mon, 12 Apr 2021 11:54:36 +0200 (CEST) DKIM-Filter: OpenDKIM Filter v2.11.0 alsa0.perex.cz 5728941 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=alsa-project.org; s=default; t=1618221326; bh=XVeqfctrDb/suB88JYJjY9egQRyna6FE4qS5h7s1WVI=; h=Date:From:To:Subject:In-Reply-To:References:Cc:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From; b=EwoD6ZtCGkA45sEmEbI+Wha1OTz7EWYDy83CIORdkIgJoE4isl/FypW37vdeqzwxO PmTcYVKdPkQt4SJc2YPaO4l/RUaEvvcGIoZJ7hnKvFGuScXbT22ewhhrTOvMSNNYph HG4S8n5Mjn0BmOTALsTCOZjKNjuQ54qPk3XsEOHI= Received: from alsa1.perex.cz (localhost.localdomain [127.0.0.1]) by alsa1.perex.cz (Postfix) with ESMTP id DB512F8025B; Mon, 12 Apr 2021 11:54:35 +0200 (CEST) Received: by alsa1.perex.cz (Postfix, from userid 50401) id 16391F80269; Mon, 12 Apr 2021 11:54:35 +0200 (CEST) Received: from mx2.suse.de (mx2.suse.de [195.135.220.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by alsa1.perex.cz (Postfix) with ESMTPS id 3A9F5F800D3 for ; Mon, 12 Apr 2021 11:54:23 +0200 (CEST) DKIM-Filter: OpenDKIM Filter v2.11.0 alsa1.perex.cz 3A9F5F800D3 X-Virus-Scanned: by amavisd-new at test-mx.suse.de Received: from relay2.suse.de (unknown [195.135.221.27]) by mx2.suse.de (Postfix) with ESMTP id 4C947AEFB; Mon, 12 Apr 2021 09:54:22 +0000 (UTC) Date: Mon, 12 Apr 2021 11:54:22 +0200 Message-ID: From: Takashi Iwai To: David Henningsson Subject: Re: [PATCH v4] sound: rawmidi: Add framing mode In-Reply-To: <20210410120229.1172054-1-coding@diwic.se> References: <20210410120229.1172054-1-coding@diwic.se> User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI/1.14.6 (Maruoka) FLIM/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL/10.8 Emacs/25.3 (x86_64-suse-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka") Content-Type: text/plain; charset=US-ASCII Cc: alsa-devel@alsa-project.org X-BeenThere: alsa-devel@alsa-project.org X-Mailman-Version: 2.1.15 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: , Errors-To: alsa-devel-bounces@alsa-project.org Sender: "Alsa-devel" On Sat, 10 Apr 2021 14:02:29 +0200, David Henningsson wrote: > > +struct snd_rawmidi_framing_tstamp { > + /* For now, frame_type is always 0. Midi 2.0 is expected to add new > + * types here. Applications are expected to skip unknown frame types. > + */ > + unsigned char frame_type; > + unsigned char length; /* number of valid bytes in data field */ > + unsigned char reserved[2]; > + unsigned int tv_nsec; /* nanoseconds */ > + unsigned long long tv_sec; /* seconds */ Please use u32 and u64 here instead of int and long long. We really want to be specific about the field types. > +static int receive_with_tstamp_framing(struct snd_rawmidi_substream *substream, > + const unsigned char *buffer, int src_count, struct timespec64 *tstamp) Pass const to tstamp. > +{ > + struct snd_rawmidi_runtime *runtime = substream->runtime; > + struct snd_rawmidi_framing_tstamp *dest_ptr; > + struct snd_rawmidi_framing_tstamp frame = { .tv_sec = tstamp->tv_sec, .tv_nsec = tstamp->tv_nsec }; > + > + int dest_frames = 0; > + int frame_size = sizeof(struct snd_rawmidi_framing_tstamp); > + > + if (snd_BUG_ON(runtime->hw_ptr & 0x1f || runtime->buffer_size & 0x1f || frame_size != 0x20)) The frame_size check should be rather BUILD_BUG_ON() as it's constant. And the buffer_size check is... well, not sure whether we need here. snd_BUG_ON() is performed even if there is no debug option set (but the debug message is suppressed). > + return -EINVAL; > + while (src_count > 0) { > + if ((int)(runtime->buffer_size - runtime->avail) < frame_size) { > + runtime->xruns += src_count; > + return dest_frames * frame_size; Better to break the loop for unifying the exit path. > @@ -987,8 +1035,15 @@ int snd_rawmidi_receive(struct snd_rawmidi_substream *substream, > "snd_rawmidi_receive: input is not active!!!\n"); > return -EINVAL; > } > - spin_lock_irqsave(&runtime->lock, flags); > - if (count == 1) { /* special case, faster code */ > + if (substream->framing == SNDRV_RAWMIDI_FRAMING_TSTAMP) { > + if (substream->clock_type == CLOCK_MONOTONIC_RAW) > + ktime_get_raw_ts64(&ts64); > + else > + ktime_get_ts64(&ts64); > + spin_lock_irqsave(&runtime->lock, flags); > + result = receive_with_tstamp_framing(substream, buffer, count, &ts64); > + } else if (count == 1) { /* special case, faster code */ > + spin_lock_irqsave(&runtime->lock, flags); > substream->bytes++; > if (runtime->avail < runtime->buffer_size) { > runtime->buffer[runtime->hw_ptr++] = buffer[0]; > @@ -999,6 +1054,7 @@ int snd_rawmidi_receive(struct snd_rawmidi_substream *substream, > runtime->xruns++; > } > } else { > + spin_lock_irqsave(&runtime->lock, flags); > substream->bytes += count; > count1 = runtime->buffer_size - runtime->hw_ptr; > if (count1 > count) It's error-prone to put the spin_lock_irqsave() in various places. If you want to get the timestamp outside the spinlock inevitably, just do it before the spin_lock_irqsave() line, if (substream->framing == SNDRV_RAWMIDI_FRAMING_TSTAMP) { if (substream->clock_type == CLOCK_MONOTONIC_RAW) ktime_get_raw_ts64(&ts64); else ktime_get_ts64(&ts64); } spin_lock_irqsave(&runtime->lock, flags); if (substream->framing == SNDRV_RAWMIDI_FRAMING_TSTAMP) { .... } else if (count == 1) { /* special case, faster code */ .... And, the patch misses the change in rawmidi_compat.c. It's also the reason we'd like to avoid the bit fields usage, too. (Not only that, though.) One thing I'm considering is how to inform the current framing and the timestamp mode to user-space. Currently we have only the ioctl to set the values but not to inquiry. Should we put those new information into the info or the status ioctl? If so, it might be also helpful to inform the frame byte size explicitly there, instead of assuming only a constant. thanks, Takashi