[v3,11/11] ASoC: SOF: Intel: Add Probe compress CPU DAIs
diff mbox series

Message ID 20200128104356.16570-12-cezary.rojewski@intel.com
State New
Headers show
Series
  • ASoC: SOF: Data probing
Related show

Commit Message

Cezary Rojewski Jan. 28, 2020, 10:43 a.m. UTC
Declare extraction CPU DAI as well as sof_probe_compr_ops. FE DAIs can
link against these new CPU DAI to create new compress devices.

Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
---
 sound/soc/sof/intel/hda-dai.c | 28 ++++++++++++++++++++++++++++
 sound/soc/sof/intel/hda.h     |  6 ++++++
 sound/soc/sof/pcm.c           | 11 ++++++++++-
 3 files changed, 44 insertions(+), 1 deletion(-)

Comments

Daniel Baluta Jan. 29, 2020, 8:04 a.m. UTC | #1
I'm not really happy with the changes inside pcm.c


On Tue, 2020-01-28 at 11:43 +0100, Cezary Rojewski wrote:
> --- a/sound/soc/sof/pcm.c
> +++ b/sound/soc/sof/pcm.c
> @@ -756,6 +756,15 @@ static void sof_pcm_remove(struct
> snd_soc_component *component)
>         snd_soc_tplg_component_remove(component,
> SND_SOC_TPLG_INDEX_ALL);
>  }
>  
> +#if IS_ENABLED(CONFIG_SND_SOC_SOF_DEBUG_PROBES)
> +#include "compress.h"
> +
> struct snd_compr_ops sof_compressed_ops = {+
> +       .copy           = sof_probe_compr_copy,
> +};
> +EXPORT_SYMBOL(sof_compressed_ops);
> +#endif
> +

Maybe call this structure sof_probe_compressed ops. Othwerwise you will
conflict with the real sof_compressed_ops.


>  void snd_sof_new_platform_drv(struct snd_sof_dev *sdev)
>  {
>         struct snd_soc_component_driver *pd = &sdev->plat_drv;
> @@ -775,7 +784,7 @@ void snd_sof_new_platform_drv(struct snd_sof_dev
> *sdev)
>         pd->trigger = sof_pcm_trigger;
>         pd->pointer = sof_pcm_pointer;
>  
> -#if IS_ENABLED(CONFIG_SND_SOC_SOF_COMPRESS)
> +#if IS_ENABLED(CONFIG_SND_SOC_SOF_DEBUG_PROBES)
>         pd->compr_ops = &sof_compressed_ops;
>  #endif
>         pd->pcm_construct = sof_pcm_new;
> 

Here you are breaking the non-existent yet compressed support. I would
leave:

#if IS_ENABLED(CONFIG_SND_SOC_SOF_DEBUG_PROBES)
         pd->compr_ops = &sof_compressed_ops;
#endif

and only override compr_ops if SND_SOC_SOF_DEBUG_PROBES is set like
this:


#if IS_ENABLED(CONFIG_SND_SOC_SOF_DEBUG_PROBES)
         pd->compr_ops = &sof_probe_compressed_ops;
#endif

Also does this mean we cannot support both "real" compressed sound card
and probes in the same time?
Cezary Rojewski Jan. 29, 2020, 9:24 a.m. UTC | #2
On 2020-01-29 09:04, Daniel Baluta wrote:
> I'm not really happy with the changes inside pcm.c
> 
> 
> On Tue, 2020-01-28 at 11:43 +0100, Cezary Rojewski wrote:
>> --- a/sound/soc/sof/pcm.c
>> +++ b/sound/soc/sof/pcm.c
>> @@ -756,6 +756,15 @@ static void sof_pcm_remove(struct
>> snd_soc_component *component)
>>          snd_soc_tplg_component_remove(component,
>> SND_SOC_TPLG_INDEX_ALL);
>>   }
>>   
>> +#if IS_ENABLED(CONFIG_SND_SOC_SOF_DEBUG_PROBES)
>> +#include "compress.h"
>> +
>> struct snd_compr_ops sof_compressed_ops = {+
>> +       .copy           = sof_probe_compr_copy,
>> +};
>> +EXPORT_SYMBOL(sof_compressed_ops);
>> +#endif
>> +
> 
> Maybe call this structure sof_probe_compressed ops. Othwerwise you will
> conflict with the real sof_compressed_ops.
> 
> 
>>   void snd_sof_new_platform_drv(struct snd_sof_dev *sdev)
>>   {
>>          struct snd_soc_component_driver *pd = &sdev->plat_drv;
>> @@ -775,7 +784,7 @@ void snd_sof_new_platform_drv(struct snd_sof_dev
>> *sdev)
>>          pd->trigger = sof_pcm_trigger;
>>          pd->pointer = sof_pcm_pointer;
>>   
>> -#if IS_ENABLED(CONFIG_SND_SOC_SOF_COMPRESS)
>> +#if IS_ENABLED(CONFIG_SND_SOC_SOF_DEBUG_PROBES)
>>          pd->compr_ops = &sof_compressed_ops;
>>   #endif
>>          pd->pcm_construct = sof_pcm_new;
>>
> 
> Here you are breaking the non-existent yet compressed support. I would
> leave:
> 
> #if IS_ENABLED(CONFIG_SND_SOC_SOF_DEBUG_PROBES)
>           pd->compr_ops = &sof_compressed_ops;
> #endif
> 
> and only override compr_ops if SND_SOC_SOF_DEBUG_PROBES is set like
> this:
> 
> 
> #if IS_ENABLED(CONFIG_SND_SOC_SOF_DEBUG_PROBES)
>           pd->compr_ops = &sof_probe_compressed_ops;
> #endif
> 
> Also does this mean we cannot support both "real" compressed sound card
> and probes in the same time?
> 
> 

Thanks for the review Daniel!

Tthe example you provided is not very clear to me - same condition is 
added for both assignments, but I'll try to answer your question.

Existing "sof_compressed_ops" don't even exist, as you said it yourself 
so nothing is lost. Changes within pcm.c are gated by _DEBUG_PROBES 
anyway so the solution is actually very clean.

While DAI can have different cops, platform driver cannot. I'm aware of 
the problem but till actual compress support for SOF comes out, minimal, 
probe-only changes were a better choice IMHO. After all, that's not a 
problem to make this code smarter in the future - not a farseer though, 
can't predict what you're going to add for SOF-compr :)

Czarek
Daniel Baluta Jan. 29, 2020, 9:46 a.m. UTC | #3
On Wed, 2020-01-29 at 10:24 +0100, Cezary Rojewski wrote:
> On 2020-01-29 09:04, Daniel Baluta wrote:
> > I'm not really happy with the changes inside pcm.c
> > 
> > 
> > On Tue, 2020-01-28 at 11:43 +0100, Cezary Rojewski wrote:
> > > --- a/sound/soc/sof/pcm.c
> > > +++ b/sound/soc/sof/pcm.c
> > > @@ -756,6 +756,15 @@ static void sof_pcm_remove(struct
> > > snd_soc_component *component)
> > >          snd_soc_tplg_component_remove(component,
> > > SND_SOC_TPLG_INDEX_ALL);
> > >   }
> > >   
> > > +#if IS_ENABLED(CONFIG_SND_SOC_SOF_DEBUG_PROBES)
> > > +#include "compress.h"
> > > +
> > > struct snd_compr_ops sof_compressed_ops = {+
> > > +       .copy           = sof_probe_compr_copy,
> > > +};
> > > +EXPORT_SYMBOL(sof_compressed_ops);
> > > +#endif
> > > +
> > 
> > Maybe call this structure sof_probe_compressed ops. Othwerwise you
> > will
> > conflict with the real sof_compressed_ops.
> > 
> > 
> > >   void snd_sof_new_platform_drv(struct snd_sof_dev *sdev)
> > >   {
> > >          struct snd_soc_component_driver *pd = &sdev->plat_drv;
> > > @@ -775,7 +784,7 @@ void snd_sof_new_platform_drv(struct
> > > snd_sof_dev
> > > *sdev)
> > >          pd->trigger = sof_pcm_trigger;
> > >          pd->pointer = sof_pcm_pointer;
> > >   
> > > -#if IS_ENABLED(CONFIG_SND_SOC_SOF_COMPRESS)
> > > +#if IS_ENABLED(CONFIG_SND_SOC_SOF_DEBUG_PROBES)
> > >          pd->compr_ops = &sof_compressed_ops;
> > >   #endif
> > >          pd->pcm_construct = sof_pcm_new;
> > > 
> > 
> > Here you are breaking the non-existent yet compressed support. I
> > would
> > leave:
> > 
> > #if IS_ENABLED(CONFIG_SND_SOC_SOF_DEBUG_PROBES)
> >           pd->compr_ops = &sof_compressed_ops;
> > #endif
> > 
> > and only override compr_ops if SND_SOC_SOF_DEBUG_PROBES is set like
> > this:
> > 
> > 
> > #if IS_ENABLED(CONFIG_SND_SOC_SOF_DEBUG_PROBES)
> >           pd->compr_ops = &sof_probe_compressed_ops;
> > #endif
> > 
> > Also does this mean we cannot support both "real" compressed sound
> > card
> > and probes in the same time?
> > 
> > 
> 
> Thanks for the review Daniel!
> 
> Tthe example you provided is not very clear to me - same condition
> is 
> added for both assignments, but I'll try to answer your question.
> 
> Existing "sof_compressed_ops" don't even exist, as you said it
> yourself 
> so nothing is lost. Changes within pcm.c are gated by _DEBUG_PROBES 
> anyway so the solution is actually very clean.
> 
> While DAI can have different cops, platform driver cannot. I'm aware
> of 
> the problem but till actual compress support for SOF comes out,
> minimal, 
> probe-only changes were a better choice IMHO. After all, that's not
> a 
> problem to make this code smarter in the future - not a farseer
> though, 
> can't predict what you're going to add for SOF-compr :)
> 

Indeed, we can make the code smarter later but I want to do the code
cleaner now. 

I think I had a copy paste error when providing the example.

So, my proposal is to override the platform driver compr_ops field
with probe compressed ops when CONFIG_SND_SOC_SOF_DEBUG_PROBES is set.

The code could look like this in the end:

#if IS_ENABLED(CONFIG_SND_SOC_SOF_COMPRESS)
          pd->compr_ops = &sof_compressed_ops;
#endif

/* Add a comment explaining that we are doing override in case of
probes */

#if IS_ENABLED(CONFIG_SND_SOC_SOF_DEBUG_PROBES)
          pd->compr_ops = &sof_probe_compressed_ops;
#endif

Also, I think there is no need to define the probe compressed ops
inside pcm.c

So, instead of 

#if IS_ENABLED(CONFIG_SND_SOC_SOF_DEBUG_PROBES)
#include "compress.h"

struct snd_compr_ops sof_compressed_ops = {
     .copy           = sof_probe_compr_copy,
};
EXPORT_SYMBOL(sof_compressed_ops);
#endif

We can do:
#if IS_ENABLED(CONFIG_SND_SOC_SOF_DEBUG_PROBES)
extern snd_compr_ops sof_probe_compressed_ops;
#endif

or even better include a header with the declaration.

And add the definition of sof_probe_compressed_ops would be somewhere
in the compressed probe file.
Cezary Rojewski Jan. 31, 2020, 12:34 p.m. UTC | #4
On 2020-01-29 10:46, Daniel Baluta wrote:
> On Wed, 2020-01-29 at 10:24 +0100, Cezary Rojewski wrote:
>>
>> Thanks for the review Daniel!
>>
>> Tthe example you provided is not very clear to me - same condition
>> is
>> added for both assignments, but I'll try to answer your question.
>>
>> Existing "sof_compressed_ops" don't even exist, as you said it
>> yourself
>> so nothing is lost. Changes within pcm.c are gated by _DEBUG_PROBES
>> anyway so the solution is actually very clean.
>>
>> While DAI can have different cops, platform driver cannot. I'm aware
>> of
>> the problem but till actual compress support for SOF comes out,
>> minimal,
>> probe-only changes were a better choice IMHO. After all, that's not
>> a
>> problem to make this code smarter in the future - not a farseer
>> though,
>> can't predict what you're going to add for SOF-compr :)
>>
> 
> Indeed, we can make the code smarter later but I want to do the code
> cleaner now.
> 
> I think I had a copy paste error when providing the example.
> 
> So, my proposal is to override the platform driver compr_ops field
> with probe compressed ops when CONFIG_SND_SOC_SOF_DEBUG_PROBES is set.
> 
> The code could look like this in the end:
> 
> #if IS_ENABLED(CONFIG_SND_SOC_SOF_COMPRESS)
>            pd->compr_ops = &sof_compressed_ops;
> #endif
> 
> /* Add a comment explaining that we are doing override in case of
> probes */
> 
> #if IS_ENABLED(CONFIG_SND_SOC_SOF_DEBUG_PROBES)
>            pd->compr_ops = &sof_probe_compressed_ops;
> #endif
> 
> Also, I think there is no need to define the probe compressed ops
> inside pcm.c
> 
> So, instead of
> 
> #if IS_ENABLED(CONFIG_SND_SOC_SOF_DEBUG_PROBES)
> #include "compress.h"
> 
> struct snd_compr_ops sof_compressed_ops = {
>       .copy           = sof_probe_compr_copy,
> };
> EXPORT_SYMBOL(sof_compressed_ops);
> #endif
> 
> We can do:
> #if IS_ENABLED(CONFIG_SND_SOC_SOF_DEBUG_PROBES)
> extern snd_compr_ops sof_probe_compressed_ops;
> #endif
> 
> or even better include a header with the declaration.
> 
> And add the definition of sof_probe_compressed_ops would be somewhere
> in the compressed probe file.
> 
> 

Your comments have been addressed in v4. Non-existant cops usage have 
been re-added and is now overridden by probes when enabled. 
sof_probe_compressed_ops declaration moved to compress.c file as requested.

Czarek

Patch
diff mbox series

diff --git a/sound/soc/sof/intel/hda-dai.c b/sound/soc/sof/intel/hda-dai.c
index 9c6e3f990ee3..ed5e7d2c0d43 100644
--- a/sound/soc/sof/intel/hda-dai.c
+++ b/sound/soc/sof/intel/hda-dai.c
@@ -399,6 +399,19 @@  static const struct snd_soc_dai_ops hda_link_dai_ops = {
 	.trigger = hda_link_pcm_trigger,
 	.prepare = hda_link_pcm_prepare,
 };
+
+#if IS_ENABLED(CONFIG_SND_SOC_SOF_HDA_PROBES)
+#include "../compress.h"
+
+static struct snd_soc_cdai_ops sof_probe_compr_ops = {
+	.startup	= sof_probe_compr_open,
+	.shutdown	= sof_probe_compr_free,
+	.set_params	= sof_probe_compr_set_params,
+	.trigger	= sof_probe_compr_trigger,
+	.pointer	= sof_probe_compr_pointer,
+};
+
+#endif
 #endif
 
 /*
@@ -460,5 +473,20 @@  struct snd_soc_dai_driver skl_dai[] = {
 	.name = "Alt Analog CPU DAI",
 	.ops = &hda_link_dai_ops,
 },
+#if IS_ENABLED(CONFIG_SND_SOC_SOF_HDA_PROBES)
+{
+	.name = "Probe Extraction CPU DAI",
+	.compress_new = snd_soc_new_compress,
+	.cops = &sof_probe_compr_ops,
+	.capture = {
+		.stream_name = "Probe Extraction",
+		.channels_min = 1,
+		.channels_max = 8,
+		.rates = SNDRV_PCM_RATE_48000,
+		.rate_min = 48000,
+		.rate_max = 48000,
+	},
+},
+#endif
 #endif
 };
diff --git a/sound/soc/sof/intel/hda.h b/sound/soc/sof/intel/hda.h
index 80aaf4172e34..955775c4fcda 100644
--- a/sound/soc/sof/intel/hda.h
+++ b/sound/soc/sof/intel/hda.h
@@ -349,7 +349,13 @@ 
 
 /* Number of DAIs */
 #if IS_ENABLED(CONFIG_SND_SOC_SOF_HDA)
+
+#if IS_ENABLED(CONFIG_SND_SOC_SOF_HDA_PROBES)
+#define SOF_SKL_NUM_DAIS		16
+#else
 #define SOF_SKL_NUM_DAIS		15
+#endif
+
 #else
 #define SOF_SKL_NUM_DAIS		8
 #endif
diff --git a/sound/soc/sof/pcm.c b/sound/soc/sof/pcm.c
index 314f3095c12f..d423fb404a3d 100644
--- a/sound/soc/sof/pcm.c
+++ b/sound/soc/sof/pcm.c
@@ -756,6 +756,15 @@  static void sof_pcm_remove(struct snd_soc_component *component)
 	snd_soc_tplg_component_remove(component, SND_SOC_TPLG_INDEX_ALL);
 }
 
+#if IS_ENABLED(CONFIG_SND_SOC_SOF_DEBUG_PROBES)
+#include "compress.h"
+
+struct snd_compr_ops sof_compressed_ops = {
+	.copy		= sof_probe_compr_copy,
+};
+EXPORT_SYMBOL(sof_compressed_ops);
+#endif
+
 void snd_sof_new_platform_drv(struct snd_sof_dev *sdev)
 {
 	struct snd_soc_component_driver *pd = &sdev->plat_drv;
@@ -775,7 +784,7 @@  void snd_sof_new_platform_drv(struct snd_sof_dev *sdev)
 	pd->trigger = sof_pcm_trigger;
 	pd->pointer = sof_pcm_pointer;
 
-#if IS_ENABLED(CONFIG_SND_SOC_SOF_COMPRESS)
+#if IS_ENABLED(CONFIG_SND_SOC_SOF_DEBUG_PROBES)
 	pd->compr_ops = &sof_compressed_ops;
 #endif
 	pd->pcm_construct = sof_pcm_new;