| Message ID | 1449176942-3441-5-git-send-email-subhransu.s.prusty@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 03 Dec 2015 22:08:53 +0100, Subhransu S. Prusty wrote: > > Setting the constraint format based on ELD was missing bit in > the sound/core pcm drm. Added with this patch. No, you can't define these here. The format really depends on the hardware, while the rate and the channels are independent. How do you know it's little-endian? And why it must be S24_LE for 24bit, not S32_LE? Takashi
On Fri, 04 Dec 2015 12:08:26 +0100, Subhransu S. Prusty wrote: > > On Thu, Dec 03, 2015 at 04:57:14PM +0100, Takashi Iwai wrote: > > On Thu, 03 Dec 2015 22:08:53 +0100, > > Subhransu S. Prusty wrote: > > > > > > Setting the constraint format based on ELD was missing bit in > > > the sound/core pcm drm. Added with this patch. > > > > No, you can't define these here. The format really depends on the > > hardware, while the rate and the channels are independent. > > Probably then I will move this definition to driver. > > > How do you know it's little-endian? And why it must be S24_LE for > > 24bit, not S32_LE? > > Regarding the little-endian, In the driver I think I should set the > constraint for both LE and BE. And the platform as it only supports LE alone > it will set the constraint accordingly and edianness will be taken care of. > > Regarding the sample size, from short audio descriptor, the samples can be > one of 16/20/24 bit. I could use the format bits for 16 and 24 bits but > don't know which format bits macro is suitable for 20bits. So kept it as > S32_LE for 20bits. Should I fix the format bits for 20bits to use S24? No you seem misunderstanding the concept... Takashi
On Thu, Dec 03, 2015 at 04:57:14PM +0100, Takashi Iwai wrote: > On Thu, 03 Dec 2015 22:08:53 +0100, > Subhransu S. Prusty wrote: > > > > Setting the constraint format based on ELD was missing bit in > > the sound/core pcm drm. Added with this patch. > > No, you can't define these here. The format really depends on the > hardware, while the rate and the channels are independent. Probably then I will move this definition to driver. > How do you know it's little-endian? And why it must be S24_LE for > 24bit, not S32_LE? Regarding the little-endian, In the driver I think I should set the constraint for both LE and BE. And the platform as it only supports LE alone it will set the constraint accordingly and edianness will be taken care of. Regarding the sample size, from short audio descriptor, the samples can be one of 16/20/24 bit. I could use the format bits for 16 and 24 bits but don't know which format bits macro is suitable for 20bits. So kept it as S32_LE for 20bits. Should I fix the format bits for 20bits to use S24? Regards, Subhransu > > > Takashi
On Sat, 05 Dec 2015 12:27:03 +0100, Subhransu S. Prusty wrote: > > On Fri, Dec 04, 2015 at 06:59:19AM +0100, Takashi Iwai wrote: > > On Fri, 04 Dec 2015 12:08:26 +0100, > > Subhransu S. Prusty wrote: > > > > > > On Thu, Dec 03, 2015 at 04:57:14PM +0100, Takashi Iwai wrote: > > > > On Thu, 03 Dec 2015 22:08:53 +0100, > > > > Subhransu S. Prusty wrote: > > > > > > > > > > Setting the constraint format based on ELD was missing bit in > > > > > the sound/core pcm drm. Added with this patch. > > > > > > > > No, you can't define these here. The format really depends on the > > > > hardware, while the rate and the channels are independent. > > > > > > Probably then I will move this definition to driver. > > > > > > > How do you know it's little-endian? And why it must be S24_LE for > > > > 24bit, not S32_LE? > > > > > > Regarding the little-endian, In the driver I think I should set the > > > constraint for both LE and BE. And the platform as it only supports LE alone > > > it will set the constraint accordingly and edianness will be taken care of. > > > > > > Regarding the sample size, from short audio descriptor, the samples can be > > > one of 16/20/24 bit. I could use the format bits for 16 and 24 bits but > > > don't know which format bits macro is suitable for 20bits. So kept it as > > > S32_LE for 20bits. Should I fix the format bits for 20bits to use S24? > > > > No you seem misunderstanding the concept... > > Sorry about that. > > I re-read the spec, it doesn't mention the container size for the samples. > Assuming the container will be 32 bits, then I think we should use S24_LE > for both 20 and 24 bits. You can't limit easily from the supported bits. In theory, all formats that may fit with the given bit should be set. For example, for a 20bit sample, S24, U24, S32, U32, S24_3, U24_3, S20_3, etc would match, and even for both endianess. The format type doesn't specify only the *max* bit it can pack, but also only the position of bits to be packed. For example, S24 packs max 24 bits in the lower 24 bits in a 32bit container. And, S24_3 packs max 24 bits in a 24bit container. Most of Intel chips takes the upper bits in a container, so usually they need S32 or S16 no matter how many bits are used. Takashi
On Sat, 05 Dec 2015 15:05:49 +0100, Subhransu S. Prusty wrote: > > On Sat, Dec 05, 2015 at 09:12:34AM +0100, Takashi Iwai wrote: > > On Sat, 05 Dec 2015 12:27:03 +0100, > > Subhransu S. Prusty wrote: > > > > > > On Fri, Dec 04, 2015 at 06:59:19AM +0100, Takashi Iwai wrote: > > > > On Fri, 04 Dec 2015 12:08:26 +0100, > > > > Subhransu S. Prusty wrote: > > > > > > > > > > On Thu, Dec 03, 2015 at 04:57:14PM +0100, Takashi Iwai wrote: > > > > > > On Thu, 03 Dec 2015 22:08:53 +0100, > > > > > > Subhransu S. Prusty wrote: > > > > > > > > > > > > > > Setting the constraint format based on ELD was missing bit in > > > > > > > the sound/core pcm drm. Added with this patch. > > > > > > > > > > > > No, you can't define these here. The format really depends on the > > > > > > hardware, while the rate and the channels are independent. > > > > > > > > > > Probably then I will move this definition to driver. > > > > > > > > > > > How do you know it's little-endian? And why it must be S24_LE for > > > > > > 24bit, not S32_LE? > > > > > > > > > > Regarding the little-endian, In the driver I think I should set the > > > > > constraint for both LE and BE. And the platform as it only supports LE alone > > > > > it will set the constraint accordingly and edianness will be taken care of. > > > > > > > > > > Regarding the sample size, from short audio descriptor, the samples can be > > > > > one of 16/20/24 bit. I could use the format bits for 16 and 24 bits but > > > > > don't know which format bits macro is suitable for 20bits. So kept it as > > > > > S32_LE for 20bits. Should I fix the format bits for 20bits to use S24? > > > > > > > > No you seem misunderstanding the concept... > > > > > > Sorry about that. > > > > > > I re-read the spec, it doesn't mention the container size for the samples. > > > Assuming the container will be 32 bits, then I think we should use S24_LE > > > for both 20 and 24 bits. > > > > You can't limit easily from the supported bits. In theory, all > > formats that may fit with the given bit should be set. For example, > > for a 20bit sample, S24, U24, S32, U32, S24_3, U24_3, S20_3, etc would > > match, and even for both endianess. > > > > The format type doesn't specify only the *max* bit it can pack, but > > also only the position of bits to be packed. For example, S24 packs > > max 24 bits in the lower 24 bits in a 32bit container. And, S24_3 > > packs max 24 bits in a 24bit container. Most of Intel chips takes the > > upper bits in a container, so usually they need S32 or S16 no matter > > how many bits are used. > > Thanks for the quick reply. > > I am thinking to set the format as S16 for 16bits and S32 for 20/24bits. > > But then we can set S32 for everything as well including 16bits. > > What do you recommend? In general it's better to use S16 for 16bits. The problem is that you tried to put in the generic code where you can't know the exact condition in the controller side. Actually the legacy HDA driver sets the formats constraint, too, but it at least knows that the HDA controller may support only S16_LE and S32_LE. That said, rather implement it locally instead of pcm_drm_eld.c. Takashi
On Fri, Dec 04, 2015 at 06:59:19AM +0100, Takashi Iwai wrote: > On Fri, 04 Dec 2015 12:08:26 +0100, > Subhransu S. Prusty wrote: > > > > On Thu, Dec 03, 2015 at 04:57:14PM +0100, Takashi Iwai wrote: > > > On Thu, 03 Dec 2015 22:08:53 +0100, > > > Subhransu S. Prusty wrote: > > > > > > > > Setting the constraint format based on ELD was missing bit in > > > > the sound/core pcm drm. Added with this patch. > > > > > > No, you can't define these here. The format really depends on the > > > hardware, while the rate and the channels are independent. > > > > Probably then I will move this definition to driver. > > > > > How do you know it's little-endian? And why it must be S24_LE for > > > 24bit, not S32_LE? > > > > Regarding the little-endian, In the driver I think I should set the > > constraint for both LE and BE. And the platform as it only supports LE alone > > it will set the constraint accordingly and edianness will be taken care of. > > > > Regarding the sample size, from short audio descriptor, the samples can be > > one of 16/20/24 bit. I could use the format bits for 16 and 24 bits but > > don't know which format bits macro is suitable for 20bits. So kept it as > > S32_LE for 20bits. Should I fix the format bits for 20bits to use S24? > > No you seem misunderstanding the concept... Sorry about that. I re-read the spec, it doesn't mention the container size for the samples. Assuming the container will be 32 bits, then I think we should use S24_LE for both 20 and 24 bits. Can you please help to understand which concept I got wrong here. Regards, Subhransu > > > Takashi
On Sat, Dec 05, 2015 at 09:12:34AM +0100, Takashi Iwai wrote: > On Sat, 05 Dec 2015 12:27:03 +0100, > Subhransu S. Prusty wrote: > > > > On Fri, Dec 04, 2015 at 06:59:19AM +0100, Takashi Iwai wrote: > > > On Fri, 04 Dec 2015 12:08:26 +0100, > > > Subhransu S. Prusty wrote: > > > > > > > > On Thu, Dec 03, 2015 at 04:57:14PM +0100, Takashi Iwai wrote: > > > > > On Thu, 03 Dec 2015 22:08:53 +0100, > > > > > Subhransu S. Prusty wrote: > > > > > > > > > > > > Setting the constraint format based on ELD was missing bit in > > > > > > the sound/core pcm drm. Added with this patch. > > > > > > > > > > No, you can't define these here. The format really depends on the > > > > > hardware, while the rate and the channels are independent. > > > > > > > > Probably then I will move this definition to driver. > > > > > > > > > How do you know it's little-endian? And why it must be S24_LE for > > > > > 24bit, not S32_LE? > > > > > > > > Regarding the little-endian, In the driver I think I should set the > > > > constraint for both LE and BE. And the platform as it only supports LE alone > > > > it will set the constraint accordingly and edianness will be taken care of. > > > > > > > > Regarding the sample size, from short audio descriptor, the samples can be > > > > one of 16/20/24 bit. I could use the format bits for 16 and 24 bits but > > > > don't know which format bits macro is suitable for 20bits. So kept it as > > > > S32_LE for 20bits. Should I fix the format bits for 20bits to use S24? > > > > > > No you seem misunderstanding the concept... > > > > Sorry about that. > > > > I re-read the spec, it doesn't mention the container size for the samples. > > Assuming the container will be 32 bits, then I think we should use S24_LE > > for both 20 and 24 bits. > > You can't limit easily from the supported bits. In theory, all > formats that may fit with the given bit should be set. For example, > for a 20bit sample, S24, U24, S32, U32, S24_3, U24_3, S20_3, etc would > match, and even for both endianess. > > The format type doesn't specify only the *max* bit it can pack, but > also only the position of bits to be packed. For example, S24 packs > max 24 bits in the lower 24 bits in a 32bit container. And, S24_3 > packs max 24 bits in a 24bit container. Most of Intel chips takes the > upper bits in a container, so usually they need S32 or S16 no matter > how many bits are used. Thanks for the quick reply. I am thinking to set the format as S16 for 16bits and S32 for 20/24bits. But then we can set S32 for everything as well including 16bits. What do you recommend? > > > Takashi
diff --git a/sound/core/pcm_drm_eld.c b/sound/core/pcm_drm_eld.c index e70379f..0c5653e 100644 --- a/sound/core/pcm_drm_eld.c +++ b/sound/core/pcm_drm_eld.c @@ -20,11 +20,49 @@ static const unsigned int eld_rates[] = { 192000, }; +static unsigned int sad_format(const u8 *sad) +{ + return ((sad[0] >> 0x3) & 0x1f); +} + +static unsigned int sad_sample_bits_lpcm(const u8 *sad) +{ + return (sad[2] & 7); +} + static unsigned int sad_max_channels(const u8 *sad) { return 1 + (sad[0] & 7); } +static int eld_limit_formats(struct snd_pcm_runtime *runtime, void *eld) +{ + u64 formats = SNDRV_PCM_FMTBIT_S16_LE; + int i; + const u8 *sad, *eld_buf = eld; + + sad = drm_eld_sad(eld_buf); + if (!sad) + goto format_constraint; + + for (i = drm_eld_sad_count(eld_buf); i > 0; i--, sad += 3) { + if (sad_format(sad) == 1) { /* AUDIO_CODING_TYPE_LPCM */ + + /* 20 bit */ + if (sad_sample_bits_lpcm(sad) & 0x2) + formats |= SNDRV_PCM_FMTBIT_S32_LE; + + /* 24 bit */ + if (sad_sample_bits_lpcm(sad) & 0x4) + formats |= SNDRV_PCM_FMTBIT_S24_LE; + } + } + +format_constraint: + return snd_pcm_hw_constraint_mask64(runtime, SNDRV_PCM_HW_PARAM_FORMAT, + formats); +} + static int eld_limit_rates(struct snd_pcm_hw_params *params, struct snd_pcm_hw_rule *rule) { @@ -93,7 +131,9 @@ int snd_pcm_hw_constraint_eld(struct snd_pcm_runtime *runtime, void *eld) ret = snd_pcm_hw_rule_add(runtime, 0, SNDRV_PCM_HW_PARAM_CHANNELS, eld_limit_channels, eld, SNDRV_PCM_HW_PARAM_RATE, -1); + if (ret < 0) + return ret; - return ret; + return eld_limit_formats(runtime, eld); } EXPORT_SYMBOL_GPL(snd_pcm_hw_constraint_eld);