| Message ID | 87wp06lotn.wl%kuninori.morimoto.gx@renesas.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Jan 25, 2018 at 01:18:37AM +0000, Kuninori Morimoto wrote: > soc_compr_get_component() searches 1st component which have compr_ops, > but it doesn't check compr_ops->xxx, In above case, compr_ops->open. > Is it OK ? > > For me, it looks like we can solve it by using "break" and "goto". > "goto" is quick hack for now. > For example like below (not tested). > If we can use rtd->platform, use it, and skip rtdcom loop by using "goto". > If we use rtdcom, call "break" after calling 1st callback. > soc_rtdcom_xxx() in soc-pcm.c is doing similar things. > I don't know this is good idea, or not. > > --------------- > diff --git a/sound/soc/soc-compress.c b/sound/soc/soc-compress.c > index 81232f4..a147a03 100644 > --- a/sound/soc/soc-compress.c > +++ b/sound/soc/soc-compress.c > @@ -53,6 +53,7 @@ static int soc_compr_open(struct snd_compr_stream *cstream) > platform->component.name); > goto plat_err; > } > + goto rtdcom_skip: > } > > for_each_rtdcom(rtd, rtdcom) { > @@ -72,10 +73,13 @@ static int soc_compr_open(struct snd_compr_stream *cstream) > component->name); > ret = __ret; > } > + break; > } > if (ret < 0) > goto machine_err; > > +rtdcom_skip: > + > if (rtd->dai_link->compr_ops && rtd->dai_link->compr_ops->startup) { > ret = rtd->dai_link->compr_ops->startup(cstream); > if (ret < 0) { > --------------- Yes I had considered adding in breaks as an initial fix, however, two things drove me in this direction. Firstly, it didn't feel like it made sense to be calling different compr_ops on different components. Not sure if anyone feels there might be use-cases for that? And what should happen if two components had the same callback? Which do we call, should we call both as the current code does, but then what to do with the results? In short I felt there is significantly more work to be done to support systems where we have multiple components providing compressed functionality on a single runtime, so better to not look like we support it at the moment. Secondly it feels like a lot of code duplication having those almost identical loops in every callback and this solution keeps things much neater. I will have a look through the soc-pcm stuff as well and have a think if there are any issues there as well, although that does all seem to work on my system. Thanks, Charles
Hi Charles Thank you for your feedback > Yes I had considered adding in breaks as an initial fix, however, > two things drove me in this direction. > > Firstly, it didn't feel like it made sense to be calling > different compr_ops on different components. Not sure if anyone > feels there might be use-cases for that? > > And what should happen if two components had the same > callback? Which do we call, should we call both as the current > code does, but then what to do with the results? In short I > felt there is significantly more work to be done to support > systems where we have multiple components providing compressed > functionality on a single runtime, so better to not look like we > support it at the moment. > > Secondly it feels like a lot of code duplication having those > almost identical loops in every callback and this solution keeps > things much neater. > > I will have a look through the soc-pcm stuff as well and have a > think if there are any issues there as well, although that does > all seem to work on my system. In existing drivers, the component which have compr_ops *was* originally "platform" component only. The main purpose why we want to replace platform (and codec) to component is that CPU/Codec/Platform categorize is no longer best match for modern device. Thus, if we can replace all CPU/Codec/Platform into component, every device can have every feature, with no categorize ! In other words, we don't know how many device have it. But anyway, we don't have such situation, and we don't know how to adjust to it at this point. Thus Yes !! I can 100% agree your opinion. Thank you for your help One note is that "rtd->platform" itself will be removed if all platforms were replaced. Then, soc_compr_get_component() / soc_compr_get_ops() might be merged or adjusted. Best regards --- Kuninori Morimoto
On Fri, Jan 26, 2018 at 12:33:53AM +0000, Kuninori Morimoto wrote: > In existing drivers, the component which have compr_ops *was* originally > "platform" component only. > The main purpose why we want to replace platform (and codec) to component is that > CPU/Codec/Platform categorize is no longer best match for modern device. > > Thus, if we can replace all CPU/Codec/Platform into component, every device > can have every feature, with no categorize ! > In other words, we don't know how many device have it. > Yeah I think there is quite a lot more work to get to that point though, we need to understand how we will present this to user-space. > But anyway, we don't have such situation, and we don't know how to adjust to it > at this point. > Thus Yes !! I can 100% agree your opinion. > Thank you for your help > > One note is that "rtd->platform" itself will be removed if all platforms were replaced. > Then, soc_compr_get_component() / soc_compr_get_ops() might be merged or adjusted. > Yes that was my thinking as well, once you remove that you should only need some minor refactoring to make the updates. Thanks, Charles
On Fri, Jan 26, 2018 at 10:03:51AM +0000, Charles Keepax wrote: > On Fri, Jan 26, 2018 at 12:33:53AM +0000, Kuninori Morimoto wrote: > > Thus, if we can replace all CPU/Codec/Platform into component, every device > > can have every feature, with no categorize ! > > In other words, we don't know how many device have it. > Yeah I think there is quite a lot more work to get to that point > though, we need to understand how we will present this to > user-space. I think some of this will sort itself out for the immediate future through implementation choices in that we've not yet seen a system where it'd make sense to combine multiple DAIs with compressed ops and don't seem likely to right now.
diff --git a/sound/soc/soc-compress.c b/sound/soc/soc-compress.c index 81232f4..a147a03 100644 --- a/sound/soc/soc-compress.c +++ b/sound/soc/soc-compress.c @@ -53,6 +53,7 @@ static int soc_compr_open(struct snd_compr_stream *cstream) platform->component.name); goto plat_err; } + goto rtdcom_skip: } for_each_rtdcom(rtd, rtdcom) { @@ -72,10 +73,13 @@ static int soc_compr_open(struct snd_compr_stream *cstream) component->name); ret = __ret; } + break; } if (ret < 0) goto machine_err; +rtdcom_skip: + if (rtd->dai_link->compr_ops && rtd->dai_link->compr_ops->startup) { ret = rtd->dai_link->compr_ops->startup(cstream); if (ret < 0) {