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@xxxxxxxxxxxxxxx>
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@xxxxxxxxxxxxxxx>
Signed-off-by: Ranjani Sridharan <ranjani.sridharan@xxxxxxxxxxxxxxx>
Co-developed-by: Fred Oh <fred.oh@xxxxxxxxxxxxxxx>
Signed-off-by: Fred Oh <fred.oh@xxxxxxxxxxxxxxx>
Signed-off-by: Dave Ertman <david.m.ertman@xxxxxxxxx>
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?
