Re: [alsa-devel] [PATCH v6 05/10] ASoC: Intel: mrfld: add DSP core controls
On Wed, Sep 17, 2014 at 04:25:57PM +0530, Subhransu S. Prusty wrote:
On Tue, Sep 16, 2014 at 12:30:53PM -0700, Mark Brown wrote:
This is returning with the lock still held AFAICT. I'm a bit surprised that we don't need to interact with the hardware if we've disabled everything, shouldn't this have some effect on the hardware?
Also the coding style thing with the comments again.
Will fix the locking and comment.
Regarding interaction with the driver, the slot map is cached and sent in sst_set_be_modules event. This is sent only when that particular BE is active, otherwise driver will happily cache these values. This is the reason why we don't see trigger to DSP when usermode fiddles around.
In our model only when a particular FE/BE/Mixer/Pipe is active we forward the settings and parameters, rest we keep the values in driver and forward when DAPM enables them.
I think we can add this explanation here at top of this file to help.
This doesn't really answer my concern - what happens if we're already active and making a change?
Power ops in SST takes care of the PM handling. Following comment is already added in the code which explains. "Send the command only if this call is the first enable or last disable"
Let us know if it is not clear enough.
No, that comment is orthogonal to the interaction with the DSP which is what is confusing.
On Wed, Sep 17, 2014 at 12:37:06PM -0700, Mark Brown wrote:
On Wed, Sep 17, 2014 at 04:25:57PM +0530, Subhransu S. Prusty wrote:
On Tue, Sep 16, 2014 at 12:30:53PM -0700, Mark Brown wrote:
This is returning with the lock still held AFAICT. I'm a bit surprised that we don't need to interact with the hardware if we've disabled everything, shouldn't this have some effect on the hardware?
Also the coding style thing with the comments again.
Will fix the locking and comment.
Regarding interaction with the driver, the slot map is cached and sent in sst_set_be_modules event. This is sent only when that particular BE is active, otherwise driver will happily cache these values. This is the reason why we don't see trigger to DSP when usermode fiddles around.
In our model only when a particular FE/BE/Mixer/Pipe is active we forward the settings and parameters, rest we keep the values in driver and forward when DAPM enables them.
I think we can add this explanation here at top of this file to help.
This doesn't really answer my concern - what happens if we're already active and making a change?
Since this is specfic to BE (SSP) port, the DSP FW doesnt allow us to reconfigure the slots when it is active. These will take effect next time the BE restarts.
Yes not ideal but thats something we have to live with!
Thanks
On Thu, Sep 18, 2014 at 11:42:37AM +0530, Vinod Koul wrote:
On Wed, Sep 17, 2014 at 12:37:06PM -0700, Mark Brown wrote:
This doesn't really answer my concern - what happens if we're already active and making a change?
Since this is specfic to BE (SSP) port, the DSP FW doesnt allow us to reconfigure the slots when it is active. These will take effect next time the BE restarts.
Yes not ideal but thats something we have to live with!
That's fine but in that case I would expect to see an error returned to userspace rather than just silently ignoring what it's doing until the next time we start a stream, or at the very least some sort of warning generated. Silently ignoring things isn't great especially with no comments in the code, it ends up looking like a bug.
On Thu, Sep 18, 2014 at 10:28:52AM -0700, Mark Brown wrote:
On Thu, Sep 18, 2014 at 11:42:37AM +0530, Vinod Koul wrote:
On Wed, Sep 17, 2014 at 12:37:06PM -0700, Mark Brown wrote:
This doesn't really answer my concern - what happens if we're already active and making a change?
Since this is specfic to BE (SSP) port, the DSP FW doesnt allow us to reconfigure the slots when it is active. These will take effect next time the BE restarts.
Yes not ideal but thats something we have to live with!
That's fine but in that case I would expect to see an error returned to userspace rather than just silently ignoring what it's doing until the next time we start a stream, or at the very least some sort of warning generated. Silently ignoring things isn't great especially with no comments in the code, it ends up looking like a bug.
Error to usermode wont be apt as we accept the value and due to constraint the value is applied at next BE start.
Yes definately makes sense to put a comment about this. Will update that
Thanks
On Fri, Sep 19, 2014 at 01:40:47PM +0530, Vinod Koul wrote:
On Thu, Sep 18, 2014 at 10:28:52AM -0700, Mark Brown wrote:
On Thu, Sep 18, 2014 at 11:42:37AM +0530, Vinod Koul wrote:
On Wed, Sep 17, 2014 at 12:37:06PM -0700, Mark Brown wrote:
This doesn't really answer my concern - what happens if we're already active and making a change?
Since this is specfic to BE (SSP) port, the DSP FW doesnt allow us to reconfigure the slots when it is active. These will take effect next time the BE restarts.
Yes not ideal but thats something we have to live with!
That's fine but in that case I would expect to see an error returned to userspace rather than just silently ignoring what it's doing until the next time we start a stream, or at the very least some sort of warning generated. Silently ignoring things isn't great especially with no comments in the code, it ends up looking like a bug.
Error to usermode wont be apt as we accept the value and due to constraint the value is applied at next BE start.
Yes definately makes sense to put a comment about this. Will update that
And turns out that limitation is gone so I was wrong here, thanks to Shubransu for pointing this out.
This function does indeed send the updated slot values to DSP
see this bit:
static int sst_slot_put(struct snd_kcontrol *kcontrol, struct snd_ctl_elem_value *ucontrol) { struct snd_soc_component *cmpnt =snd_soc_kcontrol_component(kcontrol); struct sst_data *drv = snd_soc_component_get_drvdata(cmpnt); struct sst_enum *e = (void *)kcontrol->private_value; int i, ret = 0; unsigned int ctl_no = e->reg; unsigned int is_tx = e->tx; unsigned int slot_channel_no; unsigned int val, mux; u8 *map;
map = is_tx ? sst_ssp_channel_map : sst_ssp_slot_map;
val = 1 << ctl_no; mux = ucontrol->value.enumerated.item[0]; if (mux > e->max - 1) { return -EINVAL; }
mutex_lock(&drv->lock); /* first clear all registers of this bit */ for (i = 0; i < e->max; i++) map[i] &= ~val;
if (mux == 0) {/* kctl set to 'none' */ mutex_unlock(&drv->lock); return 0; }
/* offset by one to take "None" into account */ slot_channel_no = mux - 1; map[slot_channel_no] |= val;
dev_dbg(cmpnt->dev, "%s %s map = %#x\n", is_tx ? "tx channel" : "rx slot", e->texts[mux], map[slot_channel_no]);
if (e->w && e->w->power) ret = sst_send_slot_map(drv); ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Here is the widget is powered up then we send the updated slot map to DSP.
So it will be updated real time now :)
On Fri, Sep 19, 2014 at 01:51:53PM +0530, Vinod Koul wrote:
if (e->w && e->w->power) ret = sst_send_slot_map(drv); ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Here is the widget is powered up then we send the updated slot map to DSP.
So it will be updated real time now :)
So in what situation might there not be a widget there?
On Mon, Sep 22, 2014 at 06:57:30PM -0700, Mark Brown wrote:
On Fri, Sep 19, 2014 at 01:51:53PM +0530, Vinod Koul wrote:
if (e->w && e->w->power) ret = sst_send_slot_map(drv); ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^Here is the widget is powered up then we send the updated slot map to DSP.
So it will be updated real time now :)
So in what situation might there not be a widget there?
This function is _put handler for the interleaver widget. This is linked to the dapm BE pipeline.
So the situation where there might not be a widget is remote.
Btw Shubransu has sent out the updated series with you feedback and style issues fixed.
Thanks
participants (2)
-
Mark Brown -
Vinod Koul