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=-5.8 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,NICE_REPLY_A, SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_1 autolearn=no 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 BD037C433E2 for ; Wed, 2 Sep 2020 13:30:20 +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 4780C2083B for ; Wed, 2 Sep 2020 13:30:20 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (1024-bit key) header.d=alsa-project.org header.i=@alsa-project.org header.b="oZcGvA7m" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 4780C2083B Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=linux.intel.com 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 97B8E1819; Wed, 2 Sep 2020 15:29:28 +0200 (CEST) DKIM-Filter: OpenDKIM Filter v2.11.0 alsa0.perex.cz 97B8E1819 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=alsa-project.org; s=default; t=1599053418; bh=RKCKynu2X2C62TaiF6K1w4VgxVA47qDrSzPgAbUxOSY=; h=Subject:From:To:References:Date:In-Reply-To:Cc:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From; b=oZcGvA7mC3kUPxW6fR3Q0YpULSxmXVYOQiIVbhjzLo4iz69WNbubQc1t9hNVs2T2U 6t6NrZR8as5UsnHB+n4OyRPXFtmd9IeLVtmstNXBZ6v1t+Rsk7qNA7th7kAwqTUk3C Fpnv6SJU7ybsty3ILcUZqiDJB3H1RAWRYmR8MIz4= Received: from alsa1.perex.cz (localhost.localdomain [127.0.0.1]) by alsa1.perex.cz (Postfix) with ESMTP id 26BECF801DA; Wed, 2 Sep 2020 15:29:28 +0200 (CEST) Received: by alsa1.perex.cz (Postfix, from userid 50401) id 027F0F80212; Wed, 2 Sep 2020 15:29:25 +0200 (CEST) Received: from mga07.intel.com (mga07.intel.com [134.134.136.100]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by alsa1.perex.cz (Postfix) with ESMTPS id 1F610F800BA; Wed, 2 Sep 2020 15:29:14 +0200 (CEST) DKIM-Filter: OpenDKIM Filter v2.11.0 alsa1.perex.cz 1F610F800BA IronPort-SDR: LGSqiLnhzHOqsAjCfS2G5Li/2TARmMkPC89INbUgkZc/NJdCUB/lvLkvofcnDXUaRV/s387Ghl /58EZi2ijfIA== X-IronPort-AV: E=McAfee;i="6000,8403,9731"; a="221599488" X-IronPort-AV: E=Sophos;i="5.76,383,1592895600"; d="scan'208,217";a="221599488" X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga004.jf.intel.com ([10.7.209.38]) by orsmga105.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 02 Sep 2020 06:29:09 -0700 IronPort-SDR: NxnZJYAn9oXnh7awe+/ONBugSZGSFeyFa8rMEKpzj+DOq7maVd1ZDeH6e8DrSDmPZdRjHWAlaQ abRfLiaL9fWQ== X-IronPort-AV: E=Sophos;i="5.76,383,1592895600"; d="scan'208,217";a="446526002" Received: from pharlozi-mobl.ger.corp.intel.com (HELO [10.213.23.197]) ([10.213.23.197]) by orsmga004-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 02 Sep 2020 06:29:06 -0700 Subject: Re: [PATCH] ALSA: hda: Refactor calculating SDnFMT according to specification From: "Harlozinski, Pawel" To: =?UTF-8?Q?Amadeusz_S=c5=82awi=c5=84ski?= , Kai Vehmanen , Takashi Iwai References: <20200824100034.3129-1-pawel.harlozinski@linux.intel.com> <2dbc0b8b-2ea3-19e5-cc19-ad2f59b213c1@linux.intel.com> Message-ID: <239a61b4-c9af-dced-96ab-933511a2869f@linux.intel.com> Date: Wed, 2 Sep 2020 15:29:04 +0200 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101 Thunderbird/68.12.0 MIME-Version: 1.0 In-Reply-To: <2dbc0b8b-2ea3-19e5-cc19-ad2f59b213c1@linux.intel.com> Content-Language: en-US Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit X-Content-Filtered-By: Mailman/MimeDel 2.1.15 Cc: alsa-devel@alsa-project.org, patch@alsa-project.org, broonie@kernel.org, lgirdwood@gmail.com, pierre-louis.bossart@linux.intel.com 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" > Hey! > > Thanks for Your input! > > I've created that patch because our validation is actually checking if > values > in SDnFMT are matching their expectations, and they've found  it > indicates 32 bits in 32 container while playing 24 bits in 32 container. > This could be fixed without touching checks of maxbps: > > switch (snd_pcm_format_width(format)) { > case 8: > val |= AC_FMT_BITS_8; > break; > case 16: > val |= AC_FMT_BITS_16; > break; > case 20: > val |= AC_FMT_BITS_20; > break; > case 24: > if (maxbps >= 24) > val |= AC_FMT_BITS_24; > else > val |= AC_FMT_BITS_20; > break; > case 32: > if (maxbps >= 32 || format == SNDRV_PCM_FORMAT_FLOAT_LE) > val |= AC_FMT_BITS_32; > else if (maxbps >= 24) > val |= AC_FMT_BITS_24; > else > val |= AC_FMT_BITS_20; > break; > default: > return 0; > } > > > I've simplified that because maxbps seems redundant here - thansk for > catching Kai! > Although reason of  usage maxbps is still not clear (at least for me). > > On 8/25/2020 10:25 AM, Takashi Iwai wrote: > >> On Mon, 24 Aug 2020 14:16:26 +0200, >> Kai Vehmanen wrote: >>> Hey, >>> >>> On Mon, 24 Aug 2020, Pawel Harlozinski wrote: >>> >>>> Set SDnFMT depending on which format was given, as maxbps only describes container size. >>> hmm, I'm not entirely sure that is correct. Usage may be a bit varied, but >>> most places in existing code, "maxbps" is treated as number of significant >>> bits, not the container size. E.g. in hdac_hda.c: >>> >>> » if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) >>> » » maxbps = dai->driver->playback.sig_bits; >>> » else >>> » » maxbps = dai->driver->capture.sig_bits; >>> >>> It would seem "maxbps" is a bit superfluous given the same information can >>> be relayed in "format" as well. But currently it's still used. E.g. if you >>> look at snd_hdac_query_supported_pcm(), if codec reports 24bit support, >>> format is always set to SNDRV_PCM_FMTBIT_S32_LE even if only 24bit are valid. > So, for me looks like place where we can align with actual format, > right ? >>> So snd_pcm_format_width() will not return the expected significant >>> bits info, but you have to use "maxbps". So original code seems correct >>> (or at least you'd need to update both places). > >> Hm, we need to check the call pattern, then. The maxbps passed to >> this function was supposed to be the value obtained from >> snd_hdac_query_supported_pcm(), i.e. the codec capability. > Here I'm also not sure if we should just "cut" format  in > snd_hdac_calc_stream_format (eg. 32 to 24) if codec does not support 32? > >> But, basically this patch wouldn't change any practical behavior. In >> the current code, snd_pcm_format_width() can be never 20 or 24, >> because the 24 and 24bit supports are also with SNDRV_PCM_FMT_S32_LE. >> That is, the cases 20 and 24 there are superfluous from the >> beginning (although the checks of maxbps are still needed >> >> Instead, what we could improve is: >> - Set up the proper msbits hw_constraint to reflect the maxbps value >> - Choose the right AC_FMT_BITS_* depending on the hw_params msbitsWe may change the query function not to return a single maxbps value >> but rather storing the raw PCM parameter value (AC_SUPPCM_*), and pass >> it at re-encoding the format value, too, if we want to make >> >> thanks, >> >> Takashi