Re: [PATCH 4/9] ASoC: Intel: Skylake: Add FE and BE hw_params handling

[Vinod Koul <vinod.koul@intel.com>]

[-- Attachment #1.1: Type: text/plain, Size: 5970 bytes --]

On Fri, Aug 14, 2015 at 10:53:10PM +0100, Mark Brown wrote:
> On Sat, Aug 08, 2015 at 01:06:19AM +0530, Subhransu S. Prusty wrote:
> 
> > +	/*
> > +	 * 16 bit is 16 bit container whereas 24 bit is in 32 bit container so
> > +	 * update bit depth accordingly
> > +	 */
> > +	if (format->valid_bit_depth == SKL_DEPTH_16BIT)
> > +		format->bit_depth = format->valid_bit_depth;
> > +	else if (format->valid_bit_depth == SKL_DEPTH_24BIT)
> > +		format->bit_depth = SKL_DEPTH_32BIT;
> > +
> 
> What if the depth is neither 16 bit nor 24 bit?  Shouldn't this just be
> a simple override for the 24 bit case?

These are the only two DSP supports, so am not sure that overriding will
work with DSP. I think we should add else and error out.

> 
> > +/*
> > + * Fill the BE gateway parameters
> > + * The BE gateway expects a blob of parameters which are kept in the ACPI
> > + * NHLT blob, so query the blob for interface type (i2s/pdm) and instance.
> > + * The port cna have multiple settings so pick based on the PCM
> > + * parameters
> > + */
> > +static int skl_tplg_be_fill_pipe_params(struct snd_soc_dai *dai,
> > +		struct skl_module_cfg *mconfig, struct skl_pipe_params *params)
> > +{
> > +	struct skl_pipe *pipe = mconfig->pipe;
> > +	struct nhlt_specific_cfg *cfg;
> > +	struct skl *skl = get_skl_ctx(dai->dev);
> > +	int link_type = skl_tplg_be_link_type(mconfig->dev_type);
> > +
> > +	memcpy(pipe->p_params, params, sizeof(*params));
> > +
> > +	/* update the blob based on virtual bus_id*/
> > +	cfg = (struct nhlt_specific_cfg *) skl_get_ep_blob(skl,
> > +			mconfig->vbus_id, link_type, params->s_fmt,
> > +			params->ch, params->s_freq, params->stream);
> 
> I don't seem to have slk_get_ep_blob() but hopefully it's returning a
> void * in which case why do we need to cast it?  In any case it needs to
> be defined in mainline before this can be applied :(

It is already merged in your tree and part of NHLT patch. And my bad, it
returns struct nhlt_specific_cfg so the cast is not required and will fix

> 
> > +	if (cfg) {
> > +		mconfig->formats_config.caps_size = cfg->size;
> > +		memcpy(mconfig->formats_config.caps, &cfg->caps, cfg->size);
> > +	} else {
> > +		dev_dbg(dai->dev, "Blob is NULL");
> > +		return -EINVAL;
> > +	}
> 
> This seems like a more serious problem than something we just dev_dbg()
> about?

Yes this should be an error as it will fail and we can't start a stream
without this, will fix. Thanks for pointing.

> 
> > +static int skl_tplg_be_set_params(struct snd_soc_dai *dai,
> > +					struct snd_soc_dapm_widget *w,
> > +					struct skl_pipe_params *params)
> > +{
> > +	if (!w->power) {
> > +		dev_dbg(dai->dev, "set params for widget=%s\n", w->name);
> > +		return skl_tplg_be_fill_pipe_params(dai, w->priv, params);
> > +	}
> > +
> > +	return -EINVAL;
> 
> Shouldn't that be -EBUSY?  The normal idiom would be to check to see if
> we were busy and error out rather than writing it as only configuing if
> not busy (which looks like an error case now).

Not EBUSY. So here we are trying to set params for the pipe. So we would
expect that pipe (widget) should be powered up by DAPM, but if that is not
the case then we are in bad state. I am thinking EIO might suit this one better

> 
> > +	list_for_each_entry(p, &w->sources, list_sink) {
> > +		if (p->connect && is_skl_dsp_widget_type(p->source) &&
> > +						 p->source->priv) {
> > +			ret = skl_tplg_be_set_params(dai, p->source, params);
> > +
> > +			if (ret < 0)
> > +				return ret;
> > +		} else {
> > +			ret = skl_tplg_be_set_src_pipe_params(dai, p->source,
> > +									 params);
> > +		}
> 
> These two cases appear to be identical apart from ignoring the error in
> the else case...

Former is skl_tplg_be_set_params whereas latter is
skl_tplg_be_set_src_pipe_params(). In former case we set the params but in
former we set for source pipe

Agree we should check return here, will add that
> 
> > +	}
> > +
> > +	return ret;
> 
> ...unless it happens to be the last entry in the list in which case we
> pay attention.
> 
> > +static int skl_tplg_be_set_sink_pipe_params(struct snd_soc_dai *dai,
> > +	struct snd_soc_dapm_widget *w, struct skl_pipe_params *params)
> > +{
> > +	struct snd_soc_dapm_path *p = NULL;
> > +	int ret = 0;
> > +
> > +	dev_dbg(dai->dev, "widget name=%s\n", w->name);
> > +
> > +	list_for_each_entry(p, &w->sinks, list_source) {
> > +		if (p->connect && is_skl_dsp_widget_type(p->sink) &&
> > +						p->sink->priv) {
> > +			ret = skl_tplg_be_set_params(dai, p->sink, params);
> > +
> > +			if (ret < 0)
> > +				return ret;
> > +		} else {
> > +			ret = skl_tplg_be_set_sink_pipe_params(dai, p->sink,
> > +									 params);
> > +		}
> > +	}
> 
> There's some more common code here and the same patterns as above...  :/

well the problem here is sink, source. Former was for source pipe using
p->source, here we have sink pipe and using p->sink.
Same here too that we either set params or for sink pipe

Jeeja did try to make this common but it becomes quite hard, if you have
a trick up your sleeve for this, am all ears :)

> 
> > +	if (params->stream == SNDRV_PCM_STREAM_PLAYBACK) {
> > +		w = dai->playback_widget;
> > +
> > +		dev_dbg(dai->dev, "Stream name=%s\n", w->name);
> > +		return skl_tplg_be_set_src_pipe_params(dai, w, params);
> > +	}
> > +
> > +	if (params->stream == SNDRV_PCM_STREAM_CAPTURE) {
> > +		w = dai->capture_widget;
> > +
> > +		dev_dbg(dai->dev, "Stream name=%s\n", w->name);
> > +		return skl_tplg_be_set_sink_pipe_params(dai, w, params);
> > +	}
> 
> Normally that'd be written as an if/else, and the only difference
> between the two cases here is which widget we pick...

Yes we can change to else. Btw should we rather do if and if else, and else
for error check, in case the value became bad. I am thinking latter.

-- 
~Vinod

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]