| Message ID | 1248958183-15015-12-git-send-email-eduardo.valentin@nokia.com (mailing list archive) |
|---|---|
| State | Awaiting Upstream, archived |
| Headers | show |
On Thu, Jul 30, 2009 at 03:49:34PM +0300, Eduardo Valentin wrote: > It adds a new sysfs file, where the user can configure the mcbsp mode to use. > If the mcbsp channel is in use, it does not allow the change. > Than in omap_pcm_open we can call the omap_mcbsp_get_opmode to get the mode, > store it, than use it to implement the different modes. Is this really something that people would want to tweak at runtime (except for test purposes)? -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jul 30, 2009 at 03:04:42PM +0200, ext Mark Brown wrote: > On Thu, Jul 30, 2009 at 03:49:34PM +0300, Eduardo Valentin wrote: > > > It adds a new sysfs file, where the user can configure the mcbsp mode to use. > > If the mcbsp channel is in use, it does not allow the change. > > Than in omap_pcm_open we can call the omap_mcbsp_get_opmode to get the mode, > > store it, than use it to implement the different modes. > > Is this really something that people would want to tweak at runtime > (except for test purposes)? Yes, test purposes, bug also, the same link could be sometime used for low-power playback while sometime for low-latency processing where as accurate as possible DMA pointer position info is needed.
On Thu, Jul 30, 2009 at 04:28:08PM +0300, Eduardo Valentin wrote: > On Thu, Jul 30, 2009 at 03:04:42PM +0200, ext Mark Brown wrote: > > Is this really something that people would want to tweak at runtime > > (except for test purposes)? > Yes, test purposes, bug also, the same link could be sometime used for low-power > playback while sometime for low-latency processing where as accurate as > possible DMA pointer position info is needed. Latency you *should* be able to infer from the application behaviour. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jul 30, 2009 at 03:47:25PM +0200, ext Mark Brown wrote: > On Thu, Jul 30, 2009 at 04:28:08PM +0300, Eduardo Valentin wrote: > > On Thu, Jul 30, 2009 at 03:04:42PM +0200, ext Mark Brown wrote: > > > > Is this really something that people would want to tweak at runtime > > > (except for test purposes)? > > > Yes, test purposes, bug also, the same link could be sometime used for low-power > > playback while sometime for low-latency processing where as accurate as > > possible DMA pointer position info is needed. > > Latency you *should* be able to infer from the application behaviour. Yes that's true. But I think using ELEMENT mode, position reports are much more accurate, which will result in more precise inferences for latencies.
On Thu, 30 Jul 2009 15:49:34 +0300 Eduardo Valentin <eduardo.valentin@nokia.com> wrote: > From: Peter Ujfalusi <peter.ujfalusi@nokia.com> > > It adds a new sysfs file, where the user can configure the mcbsp mode to use. > If the mcbsp channel is in use, it does not allow the change. > Than in omap_pcm_open we can call the omap_mcbsp_get_opmode to get the mode, > store it, than use it to implement the different modes. > ... > +/********************** McBSP DMA operating modes **************************/ > +#define MCBSP_DMA_MODE_ELEMENT 0 > +#define MCBSP_DMA_MODE_THRESHOLD 1 > +#define MCBSP_DMA_MODE_FRAME 2 ... > +static ssize_t dma_op_mode_show(struct device *dev, > + struct device_attribute *attr, char *buf) > +{ > + struct omap_mcbsp *mcbsp = dev_get_drvdata(dev); > + int dma_op_mode; > + > + spin_lock_irq(&mcbsp->lock); > + dma_op_mode = mcbsp->dma_op_mode; > + spin_unlock_irq(&mcbsp->lock); > + > + return sprintf(buf, "%d\n", dma_op_mode); It would be good to handle this as an ascii like e.g /sys/power/state does. "element", "threshold" or "frame". A number value doesn't tell much (for me when testing). > @@ -1195,12 +1266,14 @@ static int __devinit omap_mcbsp_probe (struct platform_device *pdev) > if (cpu_is_omap34xx()) { > mcbsp->max_tx_thres = max_thres(mcbsp); > mcbsp->max_rx_thres = max_thres(mcbsp); > + mcbsp->dma_op_mode = MCBSP_DMA_MODE_THRESHOLD; > if (omap_additional_add(pdev)) > dev_warn(&pdev->dev, > "Unable to create threshold controls\n"); > } else { > mcbsp->max_tx_thres = -EINVAL; > mcbsp->max_rx_thres = -EINVAL; > + mcbsp->dma_op_mode = MCBSP_DMA_MODE_ELEMENT; > } I would say the default mode for the omap34xx should be also element as it keeps the omap_pcm_pointer behavior the same than currently and avoids possible regression.
On Wednesday 05 August 2009 10:39:00 ext Jarkko Nikula wrote: > On Thu, 30 Jul 2009 15:49:34 +0300 > > Eduardo Valentin <eduardo.valentin@nokia.com> wrote: > > From: Peter Ujfalusi <peter.ujfalusi@nokia.com> > > > > + > > + return sprintf(buf, "%d\n", dma_op_mode); > > It would be good to handle this as an ascii like e.g /sys/power/state > does. "element", "threshold" or "frame". A number value doesn't tell > much (for me when testing). At least when reading it it would be nice to have asci, like "element (0)", "threshold (1)"... > > I would say the default mode for the omap34xx should be also element as > it keeps the omap_pcm_pointer behavior the same than currently and > avoids possible regression. Yes, the default mode should be the element mode in my opinion as well.
>> >> I would say the default mode for the omap34xx should be also element as >> it keeps the omap_pcm_pointer behavior the same than currently and >> avoids possible regression. > > Yes, the default mode should be the element mode in my opinion as well. > > -- > Péter Well, the element mode is fine for !McBSP2. One doesn't really loose the buffer pointer accuracy, because you can get the last DMA irq timestamp with SNDRV_PCM_IOCTL_TTSTAMP (and compare that to current time). The accuracy is not far off from the element mode? (If you read the DMA portion of TRM, I think there were some issues as well) Don't want to be piccy, but McBSP2 with element mode is more or less like a "joke"? As there's a large HW bugger, why not take advantage of it..why "bypass" it as default? For !McBSP2, it doesn't even pay to have the threshold mode at all, because the effect on PM is actually adverse - too frequent wakeups will cause more adverse net effect on PM (IIRC) @ VBAT. - Eero -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello guys, On Thu, Aug 06, 2009 at 08:15:40PM +0200, Nurkkala Eero.An (EXT-Offcode/Oulu) wrote: > > >> > >> I would say the default mode for the omap34xx should be also element as > >> it keeps the omap_pcm_pointer behavior the same than currently and > >> avoids possible regression. > > > > Yes, the default mode should be the element mode in my opinion as well. > > > > -- > > Péter > > > Well, the element mode is fine for !McBSP2. One doesn't really loose the > buffer pointer accuracy, because you can get the last DMA irq timestamp > with SNDRV_PCM_IOCTL_TTSTAMP (and compare that to current time). > The accuracy is not far off from the element mode? (If you read the DMA > portion of TRM, I think there were some issues as well) > > Don't want to be piccy, but McBSP2 with element mode is more or less like > a "joke"? As there's a large HW bugger, why not take advantage of it..why > "bypass" it as default? > > For !McBSP2, it doesn't even pay to have the threshold mode at all, because > the effect on PM is actually adverse - too frequent wakeups will cause more > adverse net effect on PM (IIRC) @ VBAT. I agree with Eero in this point. We can set mcbsp2 default op mode to threshold. I think we are even here :) two believe that must be element mode by default, while two want default threshold (at least for mcbsp2). We need another opinion. > > - Eero -- > To unsubscribe from this list: send the line "unsubscribe alsa-devel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
Sorry for the late reply. On Thu, 6 Aug 2009 20:15:40 +0200 <ext-Eero.Nurkkala@nokia.com> wrote: > Well, the element mode is fine for !McBSP2. One doesn't really loose the > buffer pointer accuracy, because you can get the last DMA irq timestamp > with SNDRV_PCM_IOCTL_TTSTAMP (and compare that to current time). > The accuracy is not far off from the element mode? (If you read the DMA > portion of TRM, I think there were some issues as well) > Well the threshold mode makes the offset returned from omap_pcm_pointer to behave gradually as the threshold size is set. This would affect a SW which is doing sub-period processing like mixing audio n samples ahead the current DMA pointer. Of course HW buffer in McBSP2 adds static latency but otherwise processing is similar compared to other ports and OMAPs. I would like to see this new threshold based transfer functionality to be integrated so that projects can take the advantage of it and helps generic PM development too but I don't want that it would cause any regression now. Later on it is easy to switch threshold based transfer to be the default for McBSP2 on OMAP3 but it's safer to keep current mode default over one kernel release. > For !McBSP2, it doesn't even pay to have the threshold mode at all, because > the effect on PM is actually adverse - too frequent wakeups will cause more > adverse net effect on PM (IIRC) @ VBAT. > This is good information and integrating these functionalities allow to collect more. Without regression of course :-)
On Tue, 2009-08-11 at 08:04 +0200, ext Jarkko Nikula wrote: > Sorry for the late reply. > > On Thu, 6 Aug 2009 20:15:40 +0200 > <ext-Eero.Nurkkala@nokia.com> wrote: > > > Well, the element mode is fine for !McBSP2. One doesn't really loose the > > buffer pointer accuracy, because you can get the last DMA irq timestamp > > with SNDRV_PCM_IOCTL_TTSTAMP (and compare that to current time). > > The accuracy is not far off from the element mode? (If you read the DMA > > portion of TRM, I think there were some issues as well) > > > Well the threshold mode makes the offset returned from omap_pcm_pointer > to behave gradually as the threshold size is set. This would affect a SW > which is doing sub-period processing like mixing audio n samples ahead > the current DMA pointer. Of course HW buffer in McBSP2 adds static > latency but otherwise processing is similar compared to other ports and > OMAPs. > > I would like to see this new threshold based transfer functionality to > be integrated so that projects can take the advantage of it and helps > generic PM development too but I don't want that it would cause any > regression now. Later on it is easy to switch threshold based transfer > to be the default for McBSP2 on OMAP3 but it's safer to keep current > mode default over one kernel release. > What regression are you addressing to? Of course in OMAP2s there could be issues I'm not aware of. But does this have any effect on them? (isn't this only OMAP3). There's regression indeed with McBSP2 as it is. The same regression is available for you, even with the element mode; See, the problem is the large HW buffer, and I think there's efforts to fix those issues; for example: http://bugzilla.gnome.org/show_bug.cgi?id=545807 That is causing the "known regression", it's not really the threshold/dma mode. (of course I'm probably not aware of all) If one wishes to perform runtime mixing of several channels etc, it's known to the men in the art that of course the McBSP buffer latency, was it in element or threshold mode, is required to be calculated so that the time until the sample is played, need to be considered. That can be done easily with either of the threshold/element mode. DMA transfers may also have delays due to DMA controller scheduling (other channels present). Thus, there's always some unpredictable, but rather tiny, delays. > > For !McBSP2, it doesn't even pay to have the threshold mode at all, because > > the effect on PM is actually adverse - too frequent wakeups will cause more > > adverse net effect on PM (IIRC) @ VBAT. > > > This is good information and integrating these functionalities allow to > collect more. Without regression of course :-) > > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 11 Aug 2009 09:18:01 +0300 Eero Nurkkala <ext-eero.nurkkala@nokia.com> wrote: > > I would like to see this new threshold based transfer functionality to > > be integrated so that projects can take the advantage of it and helps > > generic PM development too but I don't want that it would cause any > > regression now. Later on it is easy to switch threshold based transfer > > to be the default for McBSP2 on OMAP3 but it's safer to keep current > > mode default over one kernel release. > > > > What regression are you addressing to? Of course in OMAP2s there could > be issues I'm not aware of. But does this have any effect on them? > (isn't this only OMAP3). > A possible regression to a SW which is using McBSP2 on OMAP3. There many of those boards. The threshold based transfer will cause that omap_pcm_pointer will loose a bit its accuracy. Probably irrelevant but still better to play safe at least over one kernel release before making it default. Return value prints from omap_pcm_pointer while playing "aplay -f dat /dev/zero" element: 614 669 691 712 734 755 threshold: 512 512 512 1024 1024 1536
On Wed, 2009-08-12 at 13:45 +0200, ext Jarkko Nikula wrote: > > The threshold based transfer will cause that omap_pcm_pointer will > loose a bit its accuracy. Probably irrelevant but still better to play > safe at least over one kernel release before making it default. > No, it doesn't loose accuracy =) It's as accurate with both modes. The difference is, that the other does things in bursts; that's called evolution rather than regression =) That's how things will be in the future =) > Return value prints from omap_pcm_pointer while playing > "aplay -f dat /dev/zero" > > element: > 614 > 669 > 691 > 712 > 734 > 755 > > threshold: > 512 > 512 > 512 > 1024 > 1024 > 1536 > > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wednesday 12 August 2009 14:48:33 Nurkkala Eero.An (EXT-Offcode/Oulu) wrote: > On Wed, 2009-08-12 at 13:45 +0200, ext Jarkko Nikula wrote: > > The threshold based transfer will cause that omap_pcm_pointer will > > loose a bit its accuracy. Probably irrelevant but still better to play > > safe at least over one kernel release before making it default. > > No, it doesn't loose accuracy =) It's as accurate with both modes. > The difference is, that the other does things in bursts; In element mode it is kind of easy to estimate where the hardware actually in the playback case (aplay -f dat /dev/zero): omap_pcm_pointer returns 669, than the HW is around 669-512=157 (plus few samples). In threshold mode you only know that the HW is playing in between 0-512, 512-1024 somewhere. I know neither of these are accurate and these examples are quite oversimplified, but there is a difference and that difference is quite significant. > that's called evolution rather than regression =) Note that evolution can also introduce regression... > That's how things will be in the future =) I agree with you on this, since the threshold mode provides quite good power saving benefits. But on the other hand it would be still better to keep the element mode as default for at least one release cycle. If no report is coming about problems, than we can make the threshold mode for McBSP2 as the default
diff --git a/arch/arm/plat-omap/include/mach/mcbsp.h b/arch/arm/plat-omap/include/mach/mcbsp.h index b0fc675..61c55ec 100644 --- a/arch/arm/plat-omap/include/mach/mcbsp.h +++ b/arch/arm/plat-omap/include/mach/mcbsp.h @@ -275,6 +275,11 @@ #define RSYNCERREN 0x0001 #define WAKEUPEN_ALL (XRDYEN | RRDYEN) +/********************** McBSP DMA operating modes **************************/ +#define MCBSP_DMA_MODE_ELEMENT 0 +#define MCBSP_DMA_MODE_THRESHOLD 1 +#define MCBSP_DMA_MODE_FRAME 2 + /* we don't do multichannel for now */ struct omap_mcbsp_reg_cfg { u16 spcr2; @@ -405,6 +410,7 @@ struct omap_mcbsp { struct clk *iclk; struct clk *fclk; #ifdef CONFIG_ARCH_OMAP34XX + int dma_op_mode; u16 max_tx_thres; u16 max_rx_thres; #endif @@ -421,6 +427,7 @@ void omap_mcbsp_set_tx_threshold(unsigned int id, u16 threshold); void omap_mcbsp_set_rx_threshold(unsigned int id, u16 threshold); u16 omap_mcbsp_get_max_tx_threshold(unsigned int id); u16 omap_mcbsp_get_max_rx_threshold(unsigned int id); +int omap_mcbsp_get_dma_op_mode(unsigned int id); #else static inline void omap_mcbsp_set_tx_threshold(unsigned int id, u16 threshold) { } @@ -428,6 +435,7 @@ static inline void omap_mcbsp_set_rx_threshold(unsigned int id, u16 threshold) { } static inline u16 omap_mcbsp_get_max_tx_threshold(unsigned int id) { return 0; } static inline u16 omap_mcbsp_get_max_rx_threshold(unsigned int id) { return 0; } +static inline int omap_mcbsp_get_dma_op_mode(unsigned int id) { return 0; } #endif int omap_mcbsp_request(unsigned int id); void omap_mcbsp_free(unsigned int id); diff --git a/arch/arm/plat-omap/mcbsp.c b/arch/arm/plat-omap/mcbsp.c index 1b8ff9e..6c535b3 100644 --- a/arch/arm/plat-omap/mcbsp.c +++ b/arch/arm/plat-omap/mcbsp.c @@ -282,6 +282,29 @@ u16 omap_mcbsp_get_max_rx_threshold(unsigned int id) return mcbsp->max_rx_thres; } EXPORT_SYMBOL(omap_mcbsp_get_max_rx_threshold); + +/* + * omap_mcbsp_get_dma_op_mode just return the current configured + * operating mode for the mcbsp channel + */ +int omap_mcbsp_get_dma_op_mode(unsigned int id) +{ + struct omap_mcbsp *mcbsp; + int dma_op_mode; + + if (!omap_mcbsp_check_valid_id(id)) { + printk(KERN_ERR "%s: Invalid id (%u)\n", __func__, id + 1); + return -ENODEV; + } + mcbsp = id_to_mcbsp_ptr(id); + + spin_lock_irq(&mcbsp->lock); + dma_op_mode = mcbsp->dma_op_mode; + spin_unlock_irq(&mcbsp->lock); + + return dma_op_mode; +} +EXPORT_SYMBOL(omap_mcbsp_get_dma_op_mode); #endif /* @@ -1093,9 +1116,57 @@ static DEVICE_ATTR(prop, 0644, prop##_show, prop##_store); THRESHOLD_PROP_BUILDER(max_tx_thres); THRESHOLD_PROP_BUILDER(max_rx_thres); +static ssize_t dma_op_mode_show(struct device *dev, + struct device_attribute *attr, char *buf) +{ + struct omap_mcbsp *mcbsp = dev_get_drvdata(dev); + int dma_op_mode; + + spin_lock_irq(&mcbsp->lock); + dma_op_mode = mcbsp->dma_op_mode; + spin_unlock_irq(&mcbsp->lock); + + return sprintf(buf, "%d\n", dma_op_mode); +} + +static ssize_t dma_op_mode_store(struct device *dev, + struct device_attribute *attr, + const char *buf, size_t size) +{ + struct omap_mcbsp *mcbsp = dev_get_drvdata(dev); + unsigned long val; + int status; + + status = strict_strtoul(buf, 0, &val); + if (status) + return status; + + spin_lock_irq(&mcbsp->lock); + + if (!mcbsp->free) { + size = -EBUSY; + goto unlock; + } + + if (val > MCBSP_DMA_MODE_FRAME || val < MCBSP_DMA_MODE_ELEMENT) { + size = -EINVAL; + goto unlock; + } + + mcbsp->dma_op_mode = val; + +unlock: + spin_unlock_irq(&mcbsp->lock); + + return size; +} + +static DEVICE_ATTR(dma_op_mode, 0644, dma_op_mode_show, dma_op_mode_store); + static const struct attribute *additional_attrs[] = { &dev_attr_max_tx_thres.attr, &dev_attr_max_rx_thres.attr, + &dev_attr_dma_op_mode.attr, NULL, }; @@ -1195,12 +1266,14 @@ static int __devinit omap_mcbsp_probe(struct platform_device *pdev) if (cpu_is_omap34xx()) { mcbsp->max_tx_thres = max_thres(mcbsp); mcbsp->max_rx_thres = max_thres(mcbsp); + mcbsp->dma_op_mode = MCBSP_DMA_MODE_THRESHOLD; if (omap_additional_add(pdev)) dev_warn(&pdev->dev, "Unable to create threshold controls\n"); } else { mcbsp->max_tx_thres = -EINVAL; mcbsp->max_rx_thres = -EINVAL; + mcbsp->dma_op_mode = MCBSP_DMA_MODE_ELEMENT; } #endif