Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ASoC: SOF: Intel: fix HDAudio stream naming #3158

Closed

Conversation

plbossart
Copy link
Member

This is an invasive PR but hopefully one that will reduce confusion. When Intel contributors don't know how to call a variable, they use 'stream', and over the years the variable name is used with different types. It's just horrible and overdue for a clean-up before we start redoing all the HDaudio host/link management.

The convention I used is:

struct hdac_ext_stream *he_stream;
struct hdac_stream *hstream,

@plbossart plbossart requested review from kv2019i, ranj063, ujfalusi and RanderWang and removed request for kv2019i and ranj063 September 14, 2021 14:09
Copy link
Collaborator

@kv2019i kv2019i left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm neutral on this. I've become used to the fairly free mixing if streams... the compiler does catch mismatches of types. I do agree the stream/hda_stream fields of sof_intel_hda_stream are a bit of a brain twister, so maybe this is worth it.

@plbossart
Copy link
Member Author

I'm neutral on this. I've become used to the fairly free mixing if streams... the compiler does catch mismatches of types. I do agree the stream/hda_stream fields of sof_intel_hda_stream are a bit of a brain twister, so maybe this is worth it.

This is only the first pass @kv2019i. I have more changes coming to restructure the HDaudio DMA code, and I'd like to avoid carrying this confusing legacy.

The key part is that we seem to use multiple ways of accessing the same information, e.g.

       struct hdac_stream *hstream = substream->runtime->private_data;
       struct hdac_ext_stream *he_stream;

	/* get stored dma data if resuming from system suspend */
	he_stream = snd_soc_dai_get_dma_data(dai, substream);
	if (!he_stream) {
		he_stream = hda_link_stream_assign(bus, substream);
		if (!he_stream)
			return -EBUSY;

		snd_soc_dai_set_dma_data(dai, substream, (void *)he_stream);
	}

	stream_tag = hdac_stream(he_stream)->stream_tag;

given the inclusion we could use container_of() to find the he_stream from hstream, or if the dma_data is necessary then something is missing to actually document what pointers we are using.

it's just a giant mess.

@plbossart plbossart marked this pull request as ready for review September 14, 2021 15:10
The existing code maximizes confusion by using 'stream' and 'hstream'
variables of different types. Examples:

struct hdac_stream *stream;
struct hdac_ext_stream *stream;
struct hdac_stream *hstream;
struct hdac_ext_stream *hstream;

with some additional copy/paste remains:
struct hdac_ext_stream *azx_dev;

This patch suggests a consistent naming across all 'hdac_ext_stream'
functions. The convention is:

struct hdac_stream *hstream;
struct hdac_ext_stream *he_stream;

No functionality change - just renaming of variables and more
consistent indentation.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
…members

The existing code maximizes confusion by using 'stream' and 'hstream'
variables of different types, e.g:

struct hdac_stream *stream;
struct hdac_ext_stream *stream;
struct hdac_stream *hstream;
struct hdac_ext_stream *hstream;

This confusion is partly inherited from legacy code but SOF
contributors added their own creative spin, e.g.

struct hdac_ext_stream *link_dev;
struct hdac_ext_stream *dsp_stream;
struct hdac_ext_stream hda_stream;

and my personal favorite:

stream = &hda_stream->hda_stream;

This patch suggests a consistent naming across all Intel code related
to HDAudio stream management. The convention is - by hierarchical
order:

struct sof_intel_hda_stream *hda_stream;
struct hdac_ext_stream *he_stream;
struct hdac_stream *hstream;

No functionality change - just renaming of variables/members.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
@ujfalusi
Copy link
Collaborator

@plbossart, it is a welcome change to try to use consistent names for the same object. It is a pain for anyone new to the code to trace what is what.
I have taken a brief look and this looks good, I think there were only one unrelated white space change / alignment change in one of the patch.
I'll approve it tomorrow after going through the PR.

@plbossart
Copy link
Member Author

@plbossart, it is a welcome change to try to use consistent names for the same object. It is a pain for anyone new to the code to trace what is what.
I have taken a brief look and this looks good, I think there were only one unrelated white space change / alignment change in one of the patch.
I'll approve it tomorrow after going through the PR.

Thanks @ujfalusi. I've been with the SOF team since 2018 and I never took the time to look into the DMA code precisely because it's a mess for no good reason. I'd be surprised if we have more than 3 people in the team that have looked at this code in detail.

It's a constant of the universe that DMAs never get enough love and programming sequences are needlessly complicated... I really want to clean this up and go back through the sequences, some don't make any sense even after the renaming.

Copy link

@RanderWang RanderWang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, I don't how did the name space was built.
how about use " struct hdac_ext_stream *hext_stream" instead of "struct hdac_ext_stream *he_stream". I can't link he to "hdac_ext".

@plbossart
Copy link
Member Author

yes, I don't how did the name space was built.
how about use " struct hdac_ext_stream *hext_stream" instead of "struct hdac_ext_stream *he_stream". I can't link he to "hdac_ext".

thanks for the feedback @RanderWang and @ujfalusi. If there is more than one person concurring that's the start of a consensus. Will do the change later today and push again.

@plbossart
Copy link
Member Author

closing, will resubmit in a different PR with fixes first and such renames later.

@plbossart plbossart closed this Sep 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants