Read only byte kcontrol with access type as read & volatile #3306
Conversation
RDharageswari
requested review from
a team,
dbaluta,
jajanusz,
lbetlej,
lgirdwood,
mmaka1,
plbossart,
singalsu and
tlauda
as code owners
RDharageswari
force-pushed
the
Read_only_byte_kcontrol
branch
from
41f9a01 to
6f8dc2b
Compare
lgirdwood
left a comment
lgirdwood
left a comment
There was a problem hiding this comment.
Looks good, just minor items.
tools/topology/m4/bytecontrol.m4
Outdated
There was a problem hiding this comment.
ditto, just a small inline comment here (I know some of the others dont have them, but that's something we need to update fro readability)
There was a problem hiding this comment.
The comment doesnt match the number 260?
There was a problem hiding this comment.
@lgirdwood this change will make the tplg incompatible with older kernels. Should this be tied to ABI as well?
|
@RDharageswari I presume there will be a matching kernel PR, to change the behavior and force an IPC read? |
Yes Pierre, have them ready, yet to test and submit the patches |
RDharageswari
force-pushed
the
Read_only_byte_kcontrol
branch
2 times, most recently
from
6d5b4f8 to
176d743
Compare
This patch adds the support for read-only volatile byte kcontrol with access type as read and volatile to read the actual values from DSP, write-only byte kcontrol with access type as just write and read-write volatile byte control with access type as read, write and volatile. Signed-off-by: Dharageswari R <dharageswari.r@intel.com>
RDharageswari
force-pushed
the
Read_only_byte_kcontrol
branch
from
176d743 to
42b1543
Compare
lgirdwood
left a comment
lgirdwood
left a comment
There was a problem hiding this comment.
Looks good now, can you link the kernel PR URL here.
|
Hi Liam, |
|
CI has stalled - rerun. |
|
SOFCI TEST |
There was a problem hiding this comment.
@RDharageswari one last change before this is merged
RDharageswari
force-pushed
the
Read_only_byte_kcontrol
branch
from
42b1543 to
a9278e8
Compare
There was a problem hiding this comment.
dont you need the ABI check here as well?
RDharageswari
force-pushed
the
Read_only_byte_kcontrol
branch
from
a9278e8 to
adc6bb0
Compare
There was a problem hiding this comment.
Sorry to chime in late, why we need an extra read-only kcontrol if it denotes the same model configure params? can't we just modify the existed one directly to be RW-volatile?
There was a problem hiding this comment.
@keyonjie we want to keep our RW control non-volatile so that what we write and subsequently read are consistent.
There was a problem hiding this comment.
@keyonjie we want to keep our RW control non-volatile so that what we write and subsequently read are consistent.
How will the FW module use this model data, if the RW one and the Read only one are with the same meaning, we don't need to preserve the original written one consistent with what we have written. e.g. we write an volume level, and then FW update it internally, the read back of the volume should return this FW internal updated one.
There was a problem hiding this comment.
My point is adding one more kcontrol with almost the same meaning could confuse us.
There was a problem hiding this comment.
Hi Keyon,
The actual intention here is not to wake up DSP, if the intention of the user is just to read back the parameters that he has set. Eg: Hotword model.
Whereas this Read volatile control is actually going to return the real-time data, that will be helpful for the tuning purpose. In this way we can minimize the DSP wakes for reading the parameters.
There was a problem hiding this comment.
The parameters cached from that been set will mislead us, we should have only one original data (e.g. from real register or real-time data) which should be read back. In case that the cached data is different from the real-time one, the cached one makes no sense but only introducing confusion. IMHO we should delete this cached one but keep the real-time one only if it will be changed by the Firmware, reading back the misleading cached data to avoid waking up the DSP looks worse than refusing to reading it back when DSP is suspended.
There was a problem hiding this comment.
@RDharageswari you need to change this to add the first 2 controls by default and the get_params control only if the abj is >= 3.17
This patch adds the smart amplifier get parameter byte kcontrol to read the actual algorithm parameters from DSP during runtime. Signed-off-by: Dharageswari R <dharageswari.r@intel.com>
This patch checks for the access type of kcontrol and returns error only when the access type is not read or write. Signed-off-by: Dharageswari R <dharageswari.r@intel.com>
RDharageswari
force-pushed
the
Read_only_byte_kcontrol
branch
from
adc6bb0 to
e96449a
Compare
|
SOFCI TEST |
|
SOFCI TEST |
|
I'm a bit perplexed about the CI failure on ICL. Going to run again as I know kernel updates for ICL were pending. |
|
SOFCI TEST |
@lgirdwood I missed the ICL failure but there's known issue with ICL requiring more than 1 attempt for FW boot which has been fixed with thesofproject/linux#2382. |
This patch set creates a Read only byte kcontrol with access type as read & volatile. Reading values from DSM audio component is very important for speaker calibration & diagnosis.Temperature, DC resistance, Fc and Q of the speaker are always changing, and every speaker has its own unique value. DSM requires Rdc calibration for each speaker in the factory line to match speaker coil resistance with actual ambient temperature. Customer also monitors above speaker parameters to make sure if there is any temperature/excursion violation. This mandates the need for a kcontrol interface to read the "actual" data from DSP. Today we already have the read/write kcontrol, where read always returns the cached data, that was set during write, as the kcontrol read need not wake up DSP everytime. But the "actual" data is needed for tuning and hence the patch set.