| Message ID | 20220207122108.3780926-9-cezary.rojewski@intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | [01/17] ALSA: hda: Add helper macros for DSP capable devices | expand |
On 2022-02-25 2:37 AM, Pierre-Louis Bossart wrote: >> Audio DSP supports low power states i.e.: transitions between D0 and D3 >> and D0-substates in form of D0i3. That process is a combination of core > > D0i0 and D0i3? Ack. >> and IPC operations. Here, Dx and D0ix IPC handlers are added. >> >> Signed-off-by: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com> >> Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com> >> --- >> sound/soc/intel/avs/messages.c | 43 ++++++++++++++++++++++++++++++++++ >> sound/soc/intel/avs/messages.h | 16 +++++++++++++ >> 2 files changed, 59 insertions(+) >> >> diff --git a/sound/soc/intel/avs/messages.c b/sound/soc/intel/avs/messages.c >> index e870d5792a77..1b589689410f 100644 >> --- a/sound/soc/intel/avs/messages.c >> +++ b/sound/soc/intel/avs/messages.c >> @@ -347,3 +347,46 @@ int avs_ipc_get_large_config(struct avs_dev *adev, u16 module_id, u8 instance_id >> >> return 0; >> } >> + >> +int avs_ipc_set_dx(struct avs_dev *adev, u32 core_mask, bool powerup) >> +{ >> + union avs_module_msg msg = AVS_MODULE_REQUEST(SET_DX); >> + struct avs_ipc_msg request; >> + struct avs_dxstate_info dx; >> + int ret; >> + >> + dx.core_mask = core_mask; >> + dx.dx_mask = powerup ? core_mask : 0; >> + request.header = msg.val; >> + request.data = &dx; >> + request.size = sizeof(dx); >> + >> + /* >> + * SET_D0 is sent for non-main cores only while SET_D3 is used to >> + * suspend for all of them. Both cases prevent any D0I3 transitions. > > The asymmetry in the comment isn't clear. Did you mean that the main > code is in D0 when powered? Yes. There is no putting MAIN_CORE to D0 as we must be in D0 to begin with, if we're thinking about sending an IPC to the base firmware. >> + */ >> + ret = avs_dsp_send_pm_msg(adev, &request, NULL, true); >> + if (ret) >> + avs_ipc_err(adev, &request, "set dx", ret); >> + >> + return ret; >> +} >> + >> +int avs_ipc_set_d0ix(struct avs_dev *adev, bool enable_pg, bool streaming) >> +{ >> + union avs_module_msg msg = AVS_MODULE_REQUEST(SET_D0IX); >> + struct avs_ipc_msg request = {0}; >> + int ret; >> + >> + /* Wake & streaming for < cAVS 2.0 */ > > I don't how anyone outside of Intel could understand this comment. > Consider explaining what the two terms refer to. Sure, will improve the wording. >> + msg.ext.set_d0ix.wake = enable_pg; > > simplify the argument? Not sure anyone could understand what wake and > enable_pg mean. Well, CG and PG are popular shortcuts among Intel audio team and stand for clock gating and power gating respectively. 'wake' is firmware specific. I can provide a comment, but not all question are going to be answered by it. Firmware specification is the place to find the answer for most of these. > int avs_ipc_set_d0ix(struct avs_dev *adev, bool wake, bool streaming) > >> + msg.ext.set_d0ix.streaming = streaming; >> + >> + request.header = msg.val; >> + >> + ret = avs_dsp_send_pm_msg(adev, &request, NULL, false); >> + if (ret) >> + avs_ipc_err(adev, &request, "set d0ix", ret); >> + >> + return ret; >> +} >> diff --git a/sound/soc/intel/avs/messages.h b/sound/soc/intel/avs/messages.h >> index 1dabd1005327..bbdba4631b1f 100644 >> --- a/sound/soc/intel/avs/messages.h >> +++ b/sound/soc/intel/avs/messages.h >> @@ -101,6 +101,8 @@ enum avs_module_msg_type { >> AVS_MOD_LARGE_CONFIG_SET = 4, >> AVS_MOD_BIND = 5, >> AVS_MOD_UNBIND = 6, >> + AVS_MOD_SET_DX = 7, >> + AVS_MOD_SET_D0IX = 8, >> AVS_MOD_DELETE_INSTANCE = 11, >> }; >> >> @@ -137,6 +139,11 @@ union avs_module_msg { >> u32 dst_queue:3; >> u32 src_queue:3; >> } bind_unbind; >> + struct { >> + /* cAVS < 2.0 */ >> + u32 wake:1; >> + u32 streaming:1; > > you probably want to explain how a 'streaming' flag is set at the module > level? One would think all modules connected in a pipeline would need to > use the same flag value. Some of the fields are confusing and I agree, but driver has to adhere to FW expectations if it wants to be a working one. I would like to avoid judging the firmware architecture here, regardless of how confusing we think it is. 'wake' and 'streaming' fields are part of SET_D0ix message is which part of MODULE-type message interface. Base firmware is, from architecture point of view, a module of type=0 (module_id) and instance id=0 (instance_id). >> + } set_d0ix; >> } ext; >> }; >> } __packed; >> @@ -298,4 +305,13 @@ int avs_ipc_get_large_config(struct avs_dev *adev, u16 module_id, u8 instance_id >> u8 param_id, u8 *request_data, size_t request_size, >> u8 **reply_data, size_t *reply_size); >> >> +/* DSP cores and domains power management messages */ >> +struct avs_dxstate_info { >> + u32 core_mask; >> + u32 dx_mask; > > what is the convention for D0 and D3 in the mask ? which one is 0 or 1? > > Is this also handled in a hierarchical way where only the bits set in > core_mask matter? Can provide a short kernel-doc for these two, sure.
> >>> + msg.ext.set_d0ix.wake = enable_pg; >> >> simplify the argument? Not sure anyone could understand what wake and >> enable_pg mean. > > > Well, CG and PG are popular shortcuts among Intel audio team and stand > for clock gating and power gating respectively. 'wake' is firmware > specific. I can provide a comment, but not all question are going to be > answered by it. Firmware specification is the place to find the answer > for most of these. again please do not assume that anyone reviewing this code has access to the firmware specification. > >> int avs_ipc_set_d0ix(struct avs_dev *adev, bool wake, bool streaming) >> >>> + msg.ext.set_d0ix.streaming = streaming; >>> + >>> + request.header = msg.val; >>> + >>> + ret = avs_dsp_send_pm_msg(adev, &request, NULL, false); >>> + if (ret) >>> + avs_ipc_err(adev, &request, "set d0ix", ret); >>> + >>> + return ret; >>> +} >>> diff --git a/sound/soc/intel/avs/messages.h >>> b/sound/soc/intel/avs/messages.h >>> index 1dabd1005327..bbdba4631b1f 100644 >>> --- a/sound/soc/intel/avs/messages.h >>> +++ b/sound/soc/intel/avs/messages.h >>> @@ -101,6 +101,8 @@ enum avs_module_msg_type { >>> AVS_MOD_LARGE_CONFIG_SET = 4, >>> AVS_MOD_BIND = 5, >>> AVS_MOD_UNBIND = 6, >>> + AVS_MOD_SET_DX = 7, >>> + AVS_MOD_SET_D0IX = 8, >>> AVS_MOD_DELETE_INSTANCE = 11, >>> }; >>> @@ -137,6 +139,11 @@ union avs_module_msg { >>> u32 dst_queue:3; >>> u32 src_queue:3; >>> } bind_unbind; >>> + struct { >>> + /* cAVS < 2.0 */ >>> + u32 wake:1; >>> + u32 streaming:1; >> >> you probably want to explain how a 'streaming' flag is set at the module >> level? One would think all modules connected in a pipeline would need to >> use the same flag value. > > > Some of the fields are confusing and I agree, but driver has to adhere > to FW expectations if it wants to be a working one. I would like to > avoid judging the firmware architecture here, regardless of how > confusing we think it is. it's not about judging, just explaining what is expected on the firmware side and what the driver needs to do. > > 'wake' and 'streaming' fields are part of SET_D0ix message is which part > of MODULE-type message interface. Base firmware is, from architecture > point of view, a module of type=0 (module_id) and instance id=0 > (instance_id). > >>> + } set_d0ix; >>> } ext; >>> }; >>> } __packed; >>> @@ -298,4 +305,13 @@ int avs_ipc_get_large_config(struct avs_dev >>> *adev, u16 module_id, u8 instance_id >>> u8 param_id, u8 *request_data, size_t request_size, >>> u8 **reply_data, size_t *reply_size); >>> +/* DSP cores and domains power management messages */ >>> +struct avs_dxstate_info { >>> + u32 core_mask; >>> + u32 dx_mask; >> >> what is the convention for D0 and D3 in the mask ? which one is 0 or 1? >> >> Is this also handled in a hierarchical way where only the bits set in >> core_mask matter? > > > Can provide a short kernel-doc for these two, sure.
On 2022-02-25 9:46 PM, Pierre-Louis Bossart wrote: > >> >>>> + msg.ext.set_d0ix.wake = enable_pg; >>> >>> simplify the argument? Not sure anyone could understand what wake and >>> enable_pg mean. >> >> >> Well, CG and PG are popular shortcuts among Intel audio team and stand >> for clock gating and power gating respectively. 'wake' is firmware >> specific. I can provide a comment, but not all question are going to be >> answered by it. Firmware specification is the place to find the answer >> for most of these. > > again please do not assume that anyone reviewing this code has access to > the firmware specification. Provided simple description for the SET_D0IX message. >> >>> int avs_ipc_set_d0ix(struct avs_dev *adev, bool wake, bool streaming) >>> >>>> + msg.ext.set_d0ix.streaming = streaming; >>>> + >>>> + request.header = msg.val; >>>> + >>>> + ret = avs_dsp_send_pm_msg(adev, &request, NULL, false); >>>> + if (ret) >>>> + avs_ipc_err(adev, &request, "set d0ix", ret); >>>> + >>>> + return ret; >>>> +} >>>> diff --git a/sound/soc/intel/avs/messages.h >>>> b/sound/soc/intel/avs/messages.h >>>> index 1dabd1005327..bbdba4631b1f 100644 >>>> --- a/sound/soc/intel/avs/messages.h >>>> +++ b/sound/soc/intel/avs/messages.h >>>> @@ -101,6 +101,8 @@ enum avs_module_msg_type { >>>> AVS_MOD_LARGE_CONFIG_SET = 4, >>>> AVS_MOD_BIND = 5, >>>> AVS_MOD_UNBIND = 6, >>>> + AVS_MOD_SET_DX = 7, >>>> + AVS_MOD_SET_D0IX = 8, >>>> AVS_MOD_DELETE_INSTANCE = 11, >>>> }; >>>> @@ -137,6 +139,11 @@ union avs_module_msg { >>>> u32 dst_queue:3; >>>> u32 src_queue:3; >>>> } bind_unbind; >>>> + struct { >>>> + /* cAVS < 2.0 */ >>>> + u32 wake:1; >>>> + u32 streaming:1; >>> >>> you probably want to explain how a 'streaming' flag is set at the module >>> level? One would think all modules connected in a pipeline would need to >>> use the same flag value. >> >> >> Some of the fields are confusing and I agree, but driver has to adhere >> to FW expectations if it wants to be a working one. I would like to >> avoid judging the firmware architecture here, regardless of how >> confusing we think it is. > > it's not about judging, just explaining what is expected on the firmware > side and what the driver needs to do. Dropped all the "cavs < 2.0" unclear comments. >> >> 'wake' and 'streaming' fields are part of SET_D0ix message is which part >> of MODULE-type message interface. Base firmware is, from architecture >> point of view, a module of type=0 (module_id) and instance id=0 >> (instance_id). >> >>>> + } set_d0ix; >>>> } ext; >>>> }; >>>> } __packed; >>>> @@ -298,4 +305,13 @@ int avs_ipc_get_large_config(struct avs_dev >>>> *adev, u16 module_id, u8 instance_id >>>> u8 param_id, u8 *request_data, size_t request_size, >>>> u8 **reply_data, size_t *reply_size); >>>> +/* DSP cores and domains power management messages */ >>>> +struct avs_dxstate_info { >>>> + u32 core_mask; >>>> + u32 dx_mask; >>> >>> what is the convention for D0 and D3 in the mask ? which one is 0 or 1? >>> >>> Is this also handled in a hierarchical way where only the bits set in >>> core_mask matter? >> >> >> Can provide a short kernel-doc for these two, sure. Provided a short comments instead.
diff --git a/sound/soc/intel/avs/messages.c b/sound/soc/intel/avs/messages.c index e870d5792a77..1b589689410f 100644 --- a/sound/soc/intel/avs/messages.c +++ b/sound/soc/intel/avs/messages.c @@ -347,3 +347,46 @@ int avs_ipc_get_large_config(struct avs_dev *adev, u16 module_id, u8 instance_id return 0; } + +int avs_ipc_set_dx(struct avs_dev *adev, u32 core_mask, bool powerup) +{ + union avs_module_msg msg = AVS_MODULE_REQUEST(SET_DX); + struct avs_ipc_msg request; + struct avs_dxstate_info dx; + int ret; + + dx.core_mask = core_mask; + dx.dx_mask = powerup ? core_mask : 0; + request.header = msg.val; + request.data = &dx; + request.size = sizeof(dx); + + /* + * SET_D0 is sent for non-main cores only while SET_D3 is used to + * suspend for all of them. Both cases prevent any D0I3 transitions. + */ + ret = avs_dsp_send_pm_msg(adev, &request, NULL, true); + if (ret) + avs_ipc_err(adev, &request, "set dx", ret); + + return ret; +} + +int avs_ipc_set_d0ix(struct avs_dev *adev, bool enable_pg, bool streaming) +{ + union avs_module_msg msg = AVS_MODULE_REQUEST(SET_D0IX); + struct avs_ipc_msg request = {0}; + int ret; + + /* Wake & streaming for < cAVS 2.0 */ + msg.ext.set_d0ix.wake = enable_pg; + msg.ext.set_d0ix.streaming = streaming; + + request.header = msg.val; + + ret = avs_dsp_send_pm_msg(adev, &request, NULL, false); + if (ret) + avs_ipc_err(adev, &request, "set d0ix", ret); + + return ret; +} diff --git a/sound/soc/intel/avs/messages.h b/sound/soc/intel/avs/messages.h index 1dabd1005327..bbdba4631b1f 100644 --- a/sound/soc/intel/avs/messages.h +++ b/sound/soc/intel/avs/messages.h @@ -101,6 +101,8 @@ enum avs_module_msg_type { AVS_MOD_LARGE_CONFIG_SET = 4, AVS_MOD_BIND = 5, AVS_MOD_UNBIND = 6, + AVS_MOD_SET_DX = 7, + AVS_MOD_SET_D0IX = 8, AVS_MOD_DELETE_INSTANCE = 11, }; @@ -137,6 +139,11 @@ union avs_module_msg { u32 dst_queue:3; u32 src_queue:3; } bind_unbind; + struct { + /* cAVS < 2.0 */ + u32 wake:1; + u32 streaming:1; + } set_d0ix; } ext; }; } __packed; @@ -298,4 +305,13 @@ int avs_ipc_get_large_config(struct avs_dev *adev, u16 module_id, u8 instance_id u8 param_id, u8 *request_data, size_t request_size, u8 **reply_data, size_t *reply_size); +/* DSP cores and domains power management messages */ +struct avs_dxstate_info { + u32 core_mask; + u32 dx_mask; +} __packed; + +int avs_ipc_set_dx(struct avs_dev *adev, u32 core_mask, bool powerup); +int avs_ipc_set_d0ix(struct avs_dev *adev, bool enable_pg, bool streaming); + #endif /* __SOUND_SOC_INTEL_AVS_MSGS_H */