Re: [PATCH v2 05/14] ALSA: pcm: Add DRM helper to set constraint for format

From: Takashi Iwai <tiwai@suse.de>
To: "Subhransu S. Prusty" <subhransu.s.prusty@intel.com>
Cc: alsa-devel@alsa-project.org, patches.audio@intel.com,
	lgirdwood@gmail.com, dri-devel@lists.freedesktop.org,
	broonie@kernel.org, Daniel Vetter <daniel.vetter@intel.com>,
	Vinod Koul <vinod.koul@intel.com>
Subject: Re: [PATCH v2 05/14] ALSA: pcm: Add DRM helper to set constraint for format
Date: Sat, 05 Dec 2015 10:05:31 +0100	[thread overview]
Message-ID: <s5hy4d92rl0.wl-tiwai@suse.de> (raw)
In-Reply-To: <20151205140549.GB2441@subhransu-desktop>

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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel