Re: [PATCH v8 1/6] ALSA: pcm: add IEC958 channel status helper for hw_params

From: Takashi Iwai <tiwai@suse.de>
To: Jyri Sarha <jsarha@ti.com>
Cc: devicetree@vger.kernel.org, alsa-devel@alsa-project.org,
	Russell King - ARM Linux <linux@arm.linux.org.uk>,
	peter.ujfalusi@ti.com, tomi.valkeinen@ti.com,
	arnaud.pouliquen@st.com, dri-devel@lists.freedesktop.org,
	liam.r.girdwood@linux.intel.com, tony@atomide.com,
	broonie@kernel.org, bcousson@baylibre.com,
	linux-omap@vger.kernel.org
Subject: Re: [PATCH v8 1/6] ALSA: pcm: add IEC958 channel status helper for hw_params
Date: Wed, 30 Mar 2016 12:24:22 +0200	[thread overview]
Message-ID: <s5hio041cx5.wl-tiwai@suse.de> (raw)
In-Reply-To: <56FB8D90.9080200@ti.com>

On Wed, 30 Mar 2016 10:25:52 +0200,
Jyri Sarha wrote:
> 
> On 03/30/16 09:21, Takashi Iwai wrote:
> > On Tue, 29 Mar 2016 19:23:12 +0200,
> > Russell King - ARM Linux wrote:
> >>
> >> On Tue, Mar 29, 2016 at 10:54:08AM +0200, Takashi Iwai wrote:
> >>> On Thu, 17 Mar 2016 13:22:29 +0100,
> >>> Jyri Sarha wrote:
> >>>>
> >>>> Add IEC958 channel status helper that gets the audio properties from
> >>>> snd_pcm_hw_params instead of snd_pcm_runtime. This is needed to
> >>>> produce the channel status bits already in audio stream configuration
> >>>> phase.
> >>>>
> >>>> Signed-off-by: Jyri Sarha <jsarha@ti.com>
> >>>
> >>> This patch looks almost good to me, but...
> >>>
> >>>> @@ -71,6 +59,7 @@ int snd_pcm_create_iec958_consumer(struct snd_pcm_runtime *runtime, u8 *cs,
> >>>>  			     IEC958_AES4_CON_MAX_WORDLEN_24;
> >>>>  			break;
> >>>>  		case 24:
> >>>> +		case 32: /* Assume 24-bit width for 32-bit samples. */
> >>>>  			ws = IEC958_AES4_CON_WORDLEN_24_20 |
> >>>>  			     IEC958_AES4_CON_MAX_WORDLEN_24;
> >>>>  			break;
> >>>
> >>> ... this change is silently slipped in.  It should be mentioned in the
> >>> changelog, or split to another patch, as this is basically an
> >>> orthogonal change.
> >>
> >> Does it even make sense - AES doesn't have support for 32-bit samples,
> >> it can only ever truncate them down to 24-bit.
> > 
> > Well, using SNDRV_PCM_FORMAT_S32_* for 24 bit samples is seen often on
> > real devices, mostly on PCI ones.  So, adding the value 32 check there
> > itself is valid, I suppose.  It's rather due to a bad design in the
> > current API.
> > 
> 
> That also happens on SoC environment i2s interfaces for various reasons.
> For instance if the i2s bit-clock for sample-rate * 24 * 2 is not
> available, but sample-rate * 32 * 2 is.
> 
> > The actual bits should be specified hw_params.msbits field, instead.
> > But, not all drivers set this properly, unfortunately.
> > 
> 
> How about allowing the 32-bit format only if hw_params.msbits is set to
> 24 bits?

I'm thinking whether msbits is updated automatically upon hw_params
refinement, or it's just used as the constraint.  If msbits is always
set, we can refer this value reliably.

But, I guess just accepting the format width 32 would be good enough
as a start.  The driver is responsible to fill in the right data
format, and the helper doesn't care much about that.

Once when we figure out that msbits can be used reliably, we can
switch to it later.

> I can set the hw_params.msbits to 24 in ASoC hdmi-codec's hw_params,
> can't I? I could also fake the whole hw_params struct that I pass to
> snd_pcm_create_iec958_consumer_hw_params(), but would that make sense?

The setup of msbits is anyway good no matter whether we add the check
in iec958 layer.

> What ever the approach, I can of course split the 32 bit support into a
> separate subsequent patch, but for instance Beaglebone-black 48kHz
> 24-bit HDMI audio needs 32 bit format on i2s bus because of the
> available i2s bit-clocks.

Yes, splitting the 32bit support is more appropriate than hiding in a
patch to add the new helper.

Takashi
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel