[alsa-devel] ASoC: davinci-mcasp: Set rule constraints if implicit BCLK divider is used

[Takashi Iwai]

At Mon, 13 Apr 2015 16:58:20 +0300,
Dan Carpenter wrote:
> 
> On Mon, Apr 13, 2015 at 04:32:24PM +0300, Jyri Sarha wrote:
> > On 04/13/15 15:16, Dan Carpenter wrote:
> > >Thanks for looking at this.  I haven't double checked your analysis, but
> > >512 means the stack is 2k and that's too high.  I think there is
> > >supposed to be a build warning if we use more than 1k of stack in a
> > >function?
> > >
> > >It's better to allocate it with kmalloc() and free it at the end.
> > >
> > >regards,
> > >dan carpenter
> > >
> > 
> > The function may get called hundreds or even thousands of times when
> > an audio stream is started. How heavy is kmalloc/free cycle?
> > 
> > Using kmalloc just feels a bit stupid since in any remotely feasible
> > case the maximum number of channels is 8. But maybe I just should
> > not care.
> 
> I'm a total newbie to this specific so I can't really advise you.  I
> did find the limit where GCC prints a warning:
> 
> home/kisskb/slave/src/net/rds/iw_rdma.c: warning: the frame size of 1052 bytes is larger than 1024 bytes [-Wframe-larger-than=]: 200:1 =>
> 
> Since this could go up to 2k that's pretty huge.  One idea is to just
> say:
> 
> 	int small_buffer[64];
> 	int size = ci->max - ci->min + 1;
> 	int *list = &small_buffer;
> 
> 	if (size > 64)
> 		list = kmalloc();
> 
> 	...
> 
> free:
> 	if (size > 64)
> 		kfree(list);
> 
> If you've looked at the call trees which call this function and we can't
> actually run out of space then that's probably fine too.  I don't think
> there are any arches which still use 4k stacks.

The code there needs just to calculate min and max values instead of
the whole discrete value list.  The code has really much room to
optimize.  A totally untested patch is below.


Takashi

---
diff --git a/sound/soc/davinci/davinci-mcasp.c b/sound/soc/davinci/davinci-mcasp.c
index bb4b78eada58..68a1ae801fb8 100644
--- a/sound/soc/davinci/davinci-mcasp.c
+++ b/sound/soc/davinci/davinci-mcasp.c
@@ -1095,33 +1095,30 @@ static int davinci_mcasp_hw_rule_channels(struct snd_pcm_hw_params *params,
 	int rate = params_rate(params);
 	int max_chan_per_wire = rd->mcasp->tdm_slots < ci->max ?
 		rd->mcasp->tdm_slots : ci->max;
-	unsigned int list[ci->max - ci->min + 1];
-	int c1, c, count = 0;
+	struct snd_interval range;
+	int c1;
 
+	snd_interval_any(&range);
+	range.min = range.max = 0;
 	for (c1 = ci->min; c1 <= max_chan_per_wire; c1++) {
 		uint bclk_freq = c1*sbits*rate;
 		int ppm;
 
 		davinci_mcasp_calc_clk_div(rd->mcasp, bclk_freq, &ppm);
 		if (abs(ppm) < DAVINCI_MAX_RATE_ERROR_PPM) {
+			if (!range.min)
+				range.min = c1;
+			range.max = c1;
 			/* If we can use all tdm_slots, we can put any
 			   amount of channels to remaining wires as
 			   long as they fit in. */
-			if (c1 == rd->mcasp->tdm_slots) {
-				for (c = c1; c <= rd->serializers*c1 &&
-					     c <= ci->max; c++)
-					list[count++] = c;
-			} else {
-				list[count++] = c1;
-			}
+			if (c1 == rd->mcasp->tdm_slots)
+				range.max = rd->serializers * c1;
 		}
 	}
-	dev_dbg(rd->mcasp->dev,
-		"%d possible channel counts (%d-%d) for %d Hz and %d sbits\n",
-		count, ci->min, ci->max, rate, sbits);
 
-	return snd_interval_list(hw_param_interval(params, rule->var),
-				 count, list, 0);
+	return snd_interval_refine(hw_param_interval(params, rule->var),
+				   &range);
 }
 
 static int davinci_mcasp_startup(struct snd_pcm_substream *substream,