| Message ID | 1438976184-6160-4-git-send-email-subhransu.s.prusty@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sat, Aug 08, 2015 at 01:06:18AM +0530, Subhransu S. Prusty wrote: > + /* If its not SKL DSP type dont handle */ > + if (source->priv == NULL && (!is_skl_dsp_widget_type(w))) > + return 0; Why would this event be assigned to a widget that is not a suitable type, and what if it's an unsuitable widget type that happens ton have private data? I'm not sure I understand the priv check. You've also got a typo with "don't" and an extra space after the &&. > + list_for_each_entry(p, &w->sinks, list_source) { > + if (!p->connect) > + continue; Hrm. Do we handle routing changes well? > + /* Add connected path to one global list */ > + path_list->dapm_path = p; > + list_add_tail(&path_list->node, &skl->dapm_path_list); > + break; We have our own private path list here which includes the DAPM path... can't we outsource some of this to the topology and DAPM code? I'm a little fuzzy on what the list is for, it appears to be used mainly for deallocation though it's called dapm_path_list (which is a bit worrying). > +static int skl_tplg_mixer_dapm_post_pmu_event(struct snd_soc_dapm_widget *w, > + struct skl *skl) > +{ This looks a lot like the previous function, there's similar duplication in the other event functions. > + } > + if (src_pipe_started) { Missing blank line here. > +/* > + * In modelling, we assume ther will be one mixer in a pipeline. If a second > + * one is required that is created as another pipe entity. > + * The mixer is resposible for pipe management and represent a pipeline > + * instance > + */ You mean to say you assume there will be *exactly* one mixer?
On Fri, Aug 14, 2015 at 10:43:36PM +0100, Mark Brown wrote: > On Sat, Aug 08, 2015 at 01:06:18AM +0530, Subhransu S. Prusty wrote: > > > + /* If its not SKL DSP type dont handle */ > > + if (source->priv == NULL && (!is_skl_dsp_widget_type(w))) > > + return 0; > > Why would this event be assigned to a widget that is not a suitable > type, and what if it's an unsuitable widget type that happens ton have > private data? I'm not sure I understand the priv check. Yes rethinking now I don't see why this would be called for events we don't register as you pointed. I will recheck this bit and get back. > You've also got a typo with "don't" and an extra space after the &&. Oops, will fix. Thanks for pointing. > > + list_for_each_entry(p, &w->sinks, list_source) { > > + if (!p->connect) > > + continue; > > Hrm. Do we handle routing changes well? Yes we do :) This is the pmu handler for a mixer widget. Here we need to send Bind event to firmware for the paths which are connected, so if a path is not connected we don't need to send bind event so we skip that. > > > + /* Add connected path to one global list */ > > + path_list->dapm_path = p; > > + list_add_tail(&path_list->node, &skl->dapm_path_list); > > + break; > > We have our own private path list here which includes the DAPM path... > can't we outsource some of this to the topology and DAPM code? I'm a > little fuzzy on what the list is for, it appears to be used mainly for > deallocation though it's called dapm_path_list (which is a bit worrying). If you look at the usage of the dapm_path_list, it is used in pmd handler for this mixer. For a mixer there can be many paths and we need to disconnect the previously connected path. DAPM will disconnect and send event, so we wont know which paths were disconnected and thats why we keep a track if this by using this list. At PMU we keep adding and at PMD we check the path which was disconnected and send the event for that. This gives driver very quick lookup of which disconnect to send. I am not sure if we should be changing framework for this... > > +static int skl_tplg_mixer_dapm_post_pmu_event(struct snd_soc_dapm_widget *w, > > + struct skl *skl) > > +{ > > This looks a lot like the previous function, there's similar duplication > in the other event functions. Actually logic is quite different. The skl_tplg_mixer_dapm_post_pmu_event is for mixer and is supposed to bind to source pipe and start The previous one is for PGA and starts sink pipelines it is connected to and then its own. It doesn't do bind. Said that I will check and try to see if common code for checking and starting can be factored out from these into a common helper > > > + } > > + if (src_pipe_started) { > > Missing blank line here. Will fix > > > +/* > > + * In modelling, we assume ther will be one mixer in a pipeline. If a second > > + * one is required that is created as another pipe entity. > > + * The mixer is resposible for pipe management and represent a pipeline > > + * instance > > + */ > > You mean to say you assume there will be *exactly* one mixer? Yes :) and if we need another mixer then that will become another pipeline Thanks
On Sat, Aug 15, 2015 at 07:12:08PM +0530, Vinod Koul wrote: > On Fri, Aug 14, 2015 at 10:43:36PM +0100, Mark Brown wrote: > > > + list_for_each_entry(p, &w->sinks, list_source) { > > > + if (!p->connect) > > > + continue; > > Hrm. Do we handle routing changes well? > Yes we do :) > This is the pmu handler for a mixer widget. Here we need to send Bind event > to firmware for the paths which are connected, so if a path is not connected > we don't need to send bind event so we skip that. And we couldn't connect in any other way? This case is fine but I'm worrying about what happens if that connection gets made later - we won't get another PMU event since the widget is already powered. > > > + /* Add connected path to one global list */ > > > + path_list->dapm_path = p; > > > + list_add_tail(&path_list->node, &skl->dapm_path_list); > > > + break; > > We have our own private path list here which includes the DAPM path... > > can't we outsource some of this to the topology and DAPM code? I'm a > > little fuzzy on what the list is for, it appears to be used mainly for > > deallocation though it's called dapm_path_list (which is a bit worrying). > If you look at the usage of the dapm_path_list, it is used in pmd handler > for this mixer. For a mixer there can be many paths and we need to This is the sort of thing where the whole lack of documentation thing that I keep going on about becomes really important - there's a limited amount of time I can spend on any individual patch series and so things that need to be reverse engineered are just going to get queried a lot of the time (and even if they get reverse engineered the feedback is often going to be that the code needs to be clearer). > disconnect the previously connected path. DAPM will disconnect and send > event, so we wont know which paths were disconnected and thats why we keep a > track if this by using this list. At PMU we keep adding and at PMD we check > the path which was disconnected and send the event for that. > This gives driver very quick lookup of which disconnect to send. I am not > sure if we should be changing framework for this... So this is about generating a notification to say which routes are being disconnected? That seems like a fairly general thing, I can imagine that being quite common for DSPs - indeed it seems essential given that we won't always trigger any power changes on disconnection. > > > +static int skl_tplg_mixer_dapm_post_pmu_event(struct snd_soc_dapm_widget *w, > > > + struct skl *skl) > > > +{ > > This looks a lot like the previous function, there's similar duplication > > in the other event functions. > Actually logic is quite different. The skl_tplg_mixer_dapm_post_pmu_event is > for mixer and is supposed to bind to source pipe and start > The previous one is for PGA and starts sink pipelines it is connected to and > then its own. It doesn't do bind. There's certainly a lot of visual similarity there.
On Sat, Aug 15, 2015 at 07:36:38AM -0700, Mark Brown wrote: > On Sat, Aug 15, 2015 at 07:12:08PM +0530, Vinod Koul wrote: > > On Fri, Aug 14, 2015 at 10:43:36PM +0100, Mark Brown wrote: > > > > > + list_for_each_entry(p, &w->sinks, list_source) { > > > > + if (!p->connect) > > > > + continue; > > > > Hrm. Do we handle routing changes well? > > > Yes we do :) > > This is the pmu handler for a mixer widget. Here we need to send Bind event > > to firmware for the paths which are connected, so if a path is not connected > > we don't need to send bind event so we skip that. > > And we couldn't connect in any other way? This case is fine but I'm > worrying about what happens if that connection gets made later - we > won't get another PMU event since the widget is already powered. Yes you are right this won't be handled here, but will be handled in source pipe it is connecting to. The DAPM event will be called for that pipe, and there we check if source or sink pipe is already running which is clue to driver that we should do bind there rather than here I will add that part as well in event handler comments > > > > + /* Add connected path to one global list */ > > > > + path_list->dapm_path = p; > > > > + list_add_tail(&path_list->node, &skl->dapm_path_list); > > > > + break; > > > > We have our own private path list here which includes the DAPM path... > > > can't we outsource some of this to the topology and DAPM code? I'm a > > > little fuzzy on what the list is for, it appears to be used mainly for > > > deallocation though it's called dapm_path_list (which is a bit worrying). > > > If you look at the usage of the dapm_path_list, it is used in pmd handler > > for this mixer. For a mixer there can be many paths and we need to > > This is the sort of thing where the whole lack of documentation thing > that I keep going on about becomes really important - there's a limited > amount of time I can spend on any individual patch series and so things > that need to be reverse engineered are just going to get queried a lot > of the time (and even if they get reverse engineered the feedback is > often going to be that the code needs to be clearer). Okay, I did try to add comments to help understand but looks like I still have more work to do. Will try to add these details as well > > disconnect the previously connected path. DAPM will disconnect and send > > event, so we wont know which paths were disconnected and thats why we keep a > > track if this by using this list. At PMU we keep adding and at PMD we check > > the path which was disconnected and send the event for that. > > This gives driver very quick lookup of which disconnect to send. I am not > > sure if we should be changing framework for this... > > So this is about generating a notification to say which routes are being > disconnected? That seems like a fairly general thing, I can imagine > that being quite common for DSPs - indeed it seems essential given that > we won't always trigger any power changes on disconnection. Okay fair point. Would it be okay to carry this approach here for now and with later DAPM updates for next kernel cycle? > > > > > +static int skl_tplg_mixer_dapm_post_pmu_event(struct snd_soc_dapm_widget *w, > > > > + struct skl *skl) > > > > +{ > > > > This looks a lot like the previous function, there's similar duplication > > > in the other event functions. > > > Actually logic is quite different. The skl_tplg_mixer_dapm_post_pmu_event is > > for mixer and is supposed to bind to source pipe and start > > The previous one is for PGA and starts sink pipelines it is connected to and > > then its own. It doesn't do bind. > > There's certainly a lot of visual similarity there. Oh yes, they do look a lot similar...
On Sat, Aug 15, 2015 at 08:42:04PM +0530, Vinod Koul wrote: > On Sat, Aug 15, 2015 at 07:36:38AM -0700, Mark Brown wrote: > > This is the sort of thing where the whole lack of documentation thing > > that I keep going on about becomes really important - there's a limited > > amount of time I can spend on any individual patch series and so things > > that need to be reverse engineered are just going to get queried a lot > > of the time (and even if they get reverse engineered the feedback is > > often going to be that the code needs to be clearer). > Okay, I did try to add comments to help understand but looks like I still > have more work to do. Will try to add these details as well If you're adding any of this stuff it's really not obvious, all I'm seeing is very tactical stuff down in the details of the code which isn't always useful without any big picture - bear in mind that there's a lot of Intel internal abstractions talking to Intel internal abstractions none of which are terribly obvious, and of course the tendency to throw in things like the NHLT table or acronyms like mcps. Bigger changelogs would help a lot here, as would building the functionality up gradually rather than dumping large sections of code. Right now the changelogs just tend to be some fairly brief comments on the purpose of the code and don't really go into the structure or the design decisions that went into it.
diff --git a/sound/soc/intel/skylake/skl-topology.c b/sound/soc/intel/skylake/skl-topology.c index ede2825..deb5c0d 100644 --- a/sound/soc/intel/skylake/skl-topology.c +++ b/sound/soc/intel/skylake/skl-topology.c @@ -341,3 +341,414 @@ static int skl_tplg_bind_unbind_pipe_modules(struct skl_sst *ctx, } return 0; } + +/* + * in the Pre-PMU event of mixer we need to do following: + * - check the resources + * - Create the pipeline + * - Initialize the modules in pipeline + * - finally bind all modules together + */ +static int skl_tplg_mixer_dapm_pre_pmu_event(struct snd_soc_dapm_widget *w, + struct skl *skl) +{ + int ret; + struct skl_module_cfg *mconfig = w->priv; + struct skl_pipe *s_pipe = mconfig->pipe; + struct skl_sst *ctx = skl->skl_sst; + + dev_dbg(ctx->dev, "%s: widget =%s\n", __func__, w->name); + + /* check resource available */ + if (!skl_tplg_is_pipe_mcps_available(skl, mconfig)) + return -EBUSY; + + if (!skl_tplg_is_pipe_mem_available(skl, mconfig)) + return -ENOMEM; + + /* + * Create list of modules for pipe + * list contains modules from source to sink + */ + ret = skl_create_pipeline(ctx, mconfig->pipe); + if (ret < 0) + return ret; + + if (list_empty(&s_pipe->w_list)) { + ret = skl_tplg_get_pipe_widget(ctx->dev, w, s_pipe); + if (ret < 0) + return ret; + } + + /* Init all pipe modules from source to sink */ + ret = skl_tplg_init_pipe_modules(skl, s_pipe); + if (ret < 0) + return ret; + + /* Bind modules from source to sink */ + return skl_tplg_bind_unbind_pipe_modules(ctx, s_pipe, true); +} +/* + * in the Pre-PMU event of PGA we need to do following: + * - Bind to sink pipeline + * - Start sink pipeline, if not running + * - Then run current pipe + */ +static int skl_tplg_pga_dapm_pre_pmu_event(struct snd_soc_dapm_widget *w, + struct skl *skl) +{ + struct snd_soc_dapm_path *p; + struct skl_dapm_path_list *path_list; + struct snd_soc_dapm_widget *source, *sink; + struct skl_module_cfg *src_mconfig, *sink_mconfig; + struct skl_sst *ctx = skl->skl_sst; + int ret = 0; + + dev_dbg(ctx->dev, "%s: widget =%s\n", __func__, w->name); + + source = w; + + /* If its not SKL DSP type dont handle */ + if (source->priv == NULL && (!is_skl_dsp_widget_type(w))) + return 0; + + src_mconfig = source->priv; + + /* + * find which sink it is connected to, bind with the sink, + * if sink is not started, start sink pipe first, then start + * this pipe + */ + list_for_each_entry(p, &w->sinks, list_source) { + if (!p->connect) + continue; + + dev_dbg(ctx->dev, "%s: src widget=%s\n", __func__, w->name); + dev_dbg(ctx->dev, "%s: sink widget=%s\n", __func__, p->sink->name); + + if ((p->sink->priv != NULL) && is_skl_dsp_widget_type(p->sink)) { + + sink = p->sink; + src_mconfig = source->priv; + sink_mconfig = sink->priv; + + /* Bind source to sink, mixin is always source */ + ret = skl_tplg_bind_unbind_pipes(src_mconfig, + sink_mconfig, ctx, true); + if (ret) + return ret; + + /* Start sinks pipe first */ + if (sink_mconfig->pipe->state != SKL_PIPE_STARTED) { + ret = skl_run_pipe(ctx, sink_mconfig->pipe); + if (ret) + return ret; + } + + path_list = kzalloc(sizeof(struct skl_dapm_path_list), + GFP_KERNEL); + if (NULL == path_list) + return -ENOMEM; + + /* Add connected path to one global list */ + path_list->dapm_path = p; + list_add_tail(&path_list->node, &skl->dapm_path_list); + break; + } + } + + /* Start source pipe last after starting all sinks */ + ret = skl_run_pipe(ctx, src_mconfig->pipe); + if (ret) + return ret; + + return 0; +} + +/* + * in the Post-PMU event of mixer we need to do following: + * - Check if this pipe is running + * - if not, then + * - bind this pipeline to its source pipeline + * - start this pipeline + */ +static int skl_tplg_mixer_dapm_post_pmu_event(struct snd_soc_dapm_widget *w, + struct skl *skl) +{ + int ret = 0; + struct snd_soc_dapm_path *p; + struct snd_soc_dapm_widget *source, *sink; + struct skl_module_cfg *src_mconfig, *sink_mconfig; + struct skl_sst *ctx = skl->skl_sst; + int src_pipe_started = 0; + + dev_dbg(ctx->dev, "%s: widget = %s\n", __func__, w->name); + + sink = w; + + /* If this is not a SKL DSP widget, nothing to be done */ + if (sink->priv == NULL && (!is_skl_dsp_widget_type(w))) + return 0; + + sink_mconfig = sink->priv; + + /* + * If source pipe is already started, that means source is driving + * one more sink before this sink got connected, Since source is + * started, bind this sink to source and start this pipe. + */ + list_for_each_entry(p, &w->sources, list_sink) { + if (!p->connect) + continue; + dev_dbg(ctx->dev, "sink widget=%s\n", w->name); + dev_dbg(ctx->dev, "src widget=%s\n", p->source->name); + + if ((p->source->priv != NULL) && is_skl_dsp_widget_type(p->source)) { + source = p->source; + src_mconfig = source->priv; + sink_mconfig = sink->priv; + src_pipe_started = 1; + + /* + * check pipe state, then no need to bind or start the + * pipe + */ + if (src_mconfig->pipe->state != SKL_PIPE_STARTED) + src_pipe_started = 0; + } + } + if (src_pipe_started) { + ret = skl_tplg_bind_unbind_pipes(src_mconfig, sink_mconfig, + ctx, true); + if (ret) + return ret; + + return skl_run_pipe(ctx, sink_mconfig->pipe); + } + + return ret; +} + +/* + * in the Pre-PMD event of mixer we need to do following: + * - Stop the pipe + * - find the source connections + * - unbind with source pipelines if still connected + */ +static int skl_tplg_mixer_dapm_pre_pmd_event(struct snd_soc_dapm_widget *w, + struct skl *skl) +{ + struct snd_soc_dapm_widget *source, *sink; + struct skl_module_cfg *src_mconfig, *sink_mconfig; + int ret = 0, path_found = 0; + struct skl_dapm_path_list *path_list, *tmp_list; + struct skl_sst *ctx = skl->skl_sst; + + dev_dbg(ctx->dev, "%s: widget = %s\n", __func__, w->name); + + sink = w; + if (sink->priv == NULL && (!is_skl_dsp_widget_type(w))) + return 0; + + sink_mconfig = sink->priv; + /* Stop the pipe */ + ret = skl_stop_pipe(ctx, sink_mconfig->pipe); + if (ret) + return ret; + + list_for_each_entry_safe(path_list, tmp_list, &skl->dapm_path_list, node) { + if (path_list->dapm_path->sink == sink) { + dev_dbg(ctx->dev, "Path found = %s\n", path_list->dapm_path->name); + source = path_list->dapm_path->source; + src_mconfig = source->priv; + path_found = 1; + + list_del(&path_list->node); + kfree(path_list); + break; + } + } + + /* + * If path_found == 1, that means pmd for source pipe has + * not occurred, source is connected to some other sink. + * so its responsibility of sink to unbind itself from source. + */ + if (path_found) + ret = skl_tplg_bind_unbind_pipes(src_mconfig, sink_mconfig, + ctx, false); + + return ret; +} + +/* + * in the Post-PMD event of mixer we need to do following: + * - Free the mcps used + * - Free the mem used + * - Unbind the modules within the pipeline + * - Delete the pipeline (modules are not required to be explicitly + * deleted, pipeline delete is enough here + */ +static int skl_tplg_mixer_dapm_post_pmd_event(struct snd_soc_dapm_widget *w, + struct skl *skl) +{ + struct skl_module_cfg *mconfig = w->priv; + int ret = 0; + struct skl_sst *ctx = skl->skl_sst; + struct skl_pipe *s_pipe = mconfig->pipe; + + dev_dbg(ctx->dev, "%s: widget = %s\n", __func__, w->name); + + skl->resource.mcps -= mconfig->mcps; + + ret = skl_tplg_bind_unbind_pipe_modules(ctx, s_pipe, false); + if (ret < 0) + return ret; + + ret = skl_delete_pipe(ctx, mconfig->pipe); + skl->resource.mem -= mconfig->pipe->memory_pages; + + return ret; +} + +/* + * in the Post-PMD event of PGA we need to do following: + * - Free the mcps used + * - Stop the pipeline + * - In source pipe is connected, unbind with source pipelines + */ +static int skl_tplg_pga_dapm_post_pmd_event(struct snd_soc_dapm_widget *w, + struct skl *skl) +{ + struct snd_soc_dapm_widget *source, *sink; + struct skl_module_cfg *src_mconfig, *sink_mconfig; + int ret = 0, path_found = 0; + struct skl_dapm_path_list *path_list, *tmp_path_list; + struct skl_sst *ctx = skl->skl_sst; + + dev_dbg(ctx->dev, "%s: widget = %s\n", __func__, w->name); + + source = w; + if (source->priv == NULL && (!is_skl_dsp_widget_type(w))) + return 0; + + src_mconfig = source->priv; + + skl->resource.mcps -= src_mconfig->mcps; + /* Stop the pipe since this is a mixin module */ + ret = skl_stop_pipe(ctx, src_mconfig->pipe); + if (ret) + return ret; + + list_for_each_entry_safe(path_list, tmp_path_list, &skl->dapm_path_list, node) { + if (path_list->dapm_path->source == source) { + dev_dbg(ctx->dev, "Path found = %s\n", path_list->dapm_path->name); + sink = path_list->dapm_path->sink; + sink_mconfig = sink->priv; + path_found = 1; + + list_del(&path_list->node); + kfree(path_list); + break; + } + } + + /* + * This is a connecter and if path is found that means + * unbind between source and sink has not happened yet + */ + if (path_found) + ret = skl_tplg_bind_unbind_pipes(src_mconfig, sink_mconfig, + ctx, false); + + return ret; +} + +static struct skl *get_skl_ctx(struct device *dev) +{ + struct hdac_ext_bus *ebus = dev_get_drvdata(dev); + + return ebus_to_skl(ebus); +} + +/* In modelling, we assume ther will be one mixer in a pipeline. + * If mixer is not required then it is treated as static mixer aka vmixer + * with a hard path to source module + * So we dont need to check if source is started or not as hard path puts + * dependency on each other + */ +static int skl_tplg_vmixer_event(struct snd_soc_dapm_widget *w, + struct snd_kcontrol *k, int event) +{ + struct snd_soc_dapm_context *dapm = w->dapm; + struct skl *skl = get_skl_ctx(dapm->dev); + + dev_dbg(dapm->dev, "%s:widget=%s event=%d\n", __func__, w->name, event); + + switch (event) { + case SND_SOC_DAPM_PRE_PMU: + return skl_tplg_mixer_dapm_pre_pmu_event(w, skl); + + case SND_SOC_DAPM_POST_PMD: + return skl_tplg_mixer_dapm_post_pmd_event(w, skl); + } + + return 0; +} + +/* + * In modelling, we assume ther will be one mixer in a pipeline. If a second + * one is required that is created as another pipe entity. + * The mixer is resposible for pipe management and represent a pipeline + * instance + */ +static int skl_tplg_mixer_event(struct snd_soc_dapm_widget *w, + struct snd_kcontrol *k, int event) +{ + struct snd_soc_dapm_context *dapm = w->dapm; + struct skl *skl = get_skl_ctx(dapm->dev); + + dev_dbg(dapm->dev, "%s:widget=%s event=%d\n", __func__, w->name, event); + + switch (event) { + case SND_SOC_DAPM_PRE_PMU: + return skl_tplg_mixer_dapm_pre_pmu_event(w, skl); + + case SND_SOC_DAPM_POST_PMU: + return skl_tplg_mixer_dapm_post_pmu_event(w, skl); + + case SND_SOC_DAPM_PRE_PMD: + return skl_tplg_mixer_dapm_pre_pmd_event(w, skl); + + case SND_SOC_DAPM_POST_PMD: + return skl_tplg_mixer_dapm_post_pmd_event(w, skl); + } + + return 0; +} + +/* + * In modelling, we assumed rest of the modules in pipeline are PGA. But we + * are interested in last PGA (leaf PGA) in a pipeline to disconnect with + * the sink when it is running (two FE to one BE or one FE to two BE) + * scenarios + */ +static int skl_tplg_pga_event(struct snd_soc_dapm_widget *w, + struct snd_kcontrol *k, int event) + +{ + struct snd_soc_dapm_context *dapm = w->dapm; + struct skl *skl = get_skl_ctx(dapm->dev); + + dev_dbg(dapm->dev, "%s:widget=%s event=%d\n", __func__, w->name, event); + + switch (event) { + case SND_SOC_DAPM_PRE_PMU: + return skl_tplg_pga_dapm_pre_pmu_event(w, skl); + + case SND_SOC_DAPM_POST_PMD: + return skl_tplg_pga_dapm_post_pmd_event(w, skl); + } + + return 0; +}