Re: [PATCH 3/6] ASoC: SOF: Create client driver for IPC test

[Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>]

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?