Thanks for the review Greg.
On 10/1/20 8:09 AM, Greg KH wrote:
On Wed, Sep 30, 2020 at 03:50:48PM -0700, Dave Ertman wrote:
From: Ranjani Sridharan ranjani.sridharan@linux.intel.com
Create an SOF client driver for IPC flood test. This driver is used to set up the debugfs entries and the read/write ops for initiating the IPC flood test that would be used to measure the min/max/avg response times for sending IPCs to the DSP.
Reviewed-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Signed-off-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com Co-developed-by: Fred Oh fred.oh@linux.intel.com Signed-off-by: Fred Oh fred.oh@linux.intel.com Signed-off-by: Dave Ertman david.m.ertman@intel.com
Am I reading this series correct in that this is the only "user" of the new ancilicary bus/driver code?
This is the first user, and it was meant to demonstrate how the client is instantiated and communicates with hardware controlled by the parent. The next users will be 'audio probes', which provides the ability to extract/inject data into the DSP. We also want to split the existing audio cards into several pieces, e.g. the HDMI parts should really be presented as a separate card.
The other users will be networking/RDMA, which were actually the first to suggest this bus.
If so, why is it even needed? These are just debugfs files for testing, why does that need to be in a separate device? What is being "shared" here that needs multiple struct devices to handle?
confused,
The parent PCI device provides access to the DSP firmware/hardware and is in complete control of the IPC with the DSP firmware. The parent plays the role of a 'server' providing shared hardware access to multiple clients.
Why is this needed?
With the current audio solutions, we have a monolithic solution that has proven difficult to maintain. We'd really like to expose unrelated DSP functionality with different devices.
The best example is really HDMI. HDMI/DP audio interfaces are controlled by the same hardware, but are logically independent. What we end-up doing is re-adding the same solution over and over for each machine driver:
sound/soc/intel/boards$ git grep hda_dsp_hdmi_build_controls bxt_da7219_max98357a.c: return hda_dsp_hdmi_build_controls(card, component); bxt_rt298.c: return hda_dsp_hdmi_build_controls(card, component); cml_rt1011_rt5682.c: return hda_dsp_hdmi_build_controls(card, component); ehl_rt5660.c: return hda_dsp_hdmi_build_controls(card, pcm->codec_dai->component); glk_rt5682_max98357a.c: return hda_dsp_hdmi_build_controls(card, component); hda_dsp_common.c:int hda_dsp_hdmi_build_controls(struct snd_soc_card *card, hda_dsp_common.h:int hda_dsp_hdmi_build_controls(struct snd_soc_card *card, hda_dsp_common.h:static inline int hda_dsp_hdmi_build_controls(struct snd_soc_card *card, skl_hda_dsp_common.h: return hda_dsp_hdmi_build_controls(card, component); sof_da7219_max98373.c: return hda_dsp_hdmi_build_controls(card, sof_pcm512x.c: return hda_dsp_hdmi_build_controls(card, pcm->codec_dai->component); sof_rt5682.c: return hda_dsp_hdmi_build_controls(card, component); sof_sdw_hdmi.c: return hda_dsp_hdmi_build_controls(card, component);
and we also keep adding HDMI-related ASoC topology definitions for all the cards.
It would make a lot more sense if we could have ONE HDMI/DP card which is created, instead of managing HDMI/DP from the card that is supposed to deal with local accessories based on HDaudio/DMIC/SoundWire/I2S.
The audio probes are similar, we want to have a single probe client instead of adding audio probes to every single card we have to maintain.
On platforms where the DSP can deal with sensors, this would also allow the parent to expose IIO and HID clients.
Going back to this IPC test, maybe the commit message is unclear: we already have this functionality in the mainline, it's been very helpful for stress tests. What this patch shows is that moving the functionality to a client makes it possible to scale to 2 or more clients with a simple set of register/unregister. The device model makes it really easy to scale.
So yes, you are correct that for now there is a single user with very limited functionality. This is intentional to make the reviews simpler, but if/when this bus is part of the mainline we'll have additional users, and not just from Intel if you look at the reviewed-by tags.
We might even remove the platform devices used for the SoundWire master and use this instead :-)
Does this help?