| Message ID | 87y15yl1fa.wl-kuninori.morimoto.gx@renesas.com (mailing list archive) |
|---|---|
| State | RFC |
| Headers | show |
| Series | [RFC,01/xx] ALSA: add snd_stream_is_playback/capture() macro | expand |
On 7/19/2024 1:34 AM, Kuninori Morimoto wrote: > Many drivers are using below code to know the direction. > > if (direction == SNDRV_PCM_STREAM_PLAYBACK) > > Add snd_stream_is_playback/capture() macro to handle it. > It also adds snd_substream_is_playback/capture() to handle > snd_pcm_substream. > > Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> > --- > include/sound/pcm.h | 19 +++++++++++++++++++ > 1 file changed, 19 insertions(+) > > diff --git a/include/sound/pcm.h b/include/sound/pcm.h > index 3edd7a7346daa..024dc2b337154 100644 > --- a/include/sound/pcm.h > +++ b/include/sound/pcm.h > @@ -501,6 +501,25 @@ struct snd_pcm_substream { > > #define SUBSTREAM_BUSY(substream) ((substream)->ref_count > 0) > > +static inline int snd_stream_is_playback(int stream) > +{ > + return stream == SNDRV_PCM_STREAM_PLAYBACK; > +} > + > +static inline int snd_stream_is_capture(int stream) > +{ > + return stream == SNDRV_PCM_STREAM_CAPTURE; > +} > + > +static inline int snd_substream_is_playback(const struct snd_pcm_substream *substream) > +{ > + return snd_stream_is_playback(substream->stream); > +} > + > +static inline int snd_substream_is_capture(const struct snd_pcm_substream *substream) > +{ > + return snd_stream_is_capture(substream->stream); > +} > > struct snd_pcm_str { > int stream; /* stream (direction) */ Perhaps you could use generics here, so you could have one caller for both cases? Something like: #define snd_pcm_is_playback(x) _Generic((x), \ int : (x == SNDRV_PCM_STREAM_PLAYBACK), \ struct snd_pcm_substream *substream * : (x->stream == SNDRV_PCM_STREAM_PLAYBACK))(x) or just call the above functions in it?
Hi Amadeusz Thank you for comment > > Many drivers are using below code to know the direction. > > > > if (direction == SNDRV_PCM_STREAM_PLAYBACK) > > > > Add snd_stream_is_playback/capture() macro to handle it. > > It also adds snd_substream_is_playback/capture() to handle > > snd_pcm_substream. > > > > Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> > > --- (snip) > Perhaps you could use generics here, so you could have one caller for > both cases? > > Something like: > #define snd_pcm_is_playback(x) _Generic((x), \ > int : (x == SNDRV_PCM_STREAM_PLAYBACK), \ > struct snd_pcm_substream *substream * : (x->stream == > SNDRV_PCM_STREAM_PLAYBACK))(x) > or just call the above functions in it? Actually, I have tried _Generic() first, but changed to current style, because many drivers are using own direction variable, and they are using own variable types. But I think more again. Thank you for your help !! Best regards --- Kuninori Morimoto
Hi Amadeusz, Takashi > > Perhaps you could use generics here, so you could have one caller for > > both cases? > > > > Something like: > > #define snd_pcm_is_playback(x) _Generic((x), \ > > int : (x == SNDRV_PCM_STREAM_PLAYBACK), \ > > struct snd_pcm_substream *substream * : (x->stream == > > SNDRV_PCM_STREAM_PLAYBACK))(x) > > or just call the above functions in it? Hmm... I couldn't compile above inline style. I need to create function, and use it on _Generic(). And then, _Generic() is very picky for variable sytle. This means I need to create function for "int" / "const int", "unsigned int", "const unsigned int" static inline int snd_pcm_stream_is_playback_(const int stream) { return stream == SNDRV_PCM_STREAM_PLAYBACK; } static inline int snd_pcm_stream_is_playback(int stream) { return stream == SNDRV_PCM_STREAM_PLAYBACK; } static inline int snd_pcm_stream_is_playback_u(const unsigned int stream) { return stream == SNDRV_PCM_STREAM_PLAYBACK; } static inline int snd_pcm_stream_is_playbacku(unsigned int stream) { return stream == SNDRV_PCM_STREAM_PLAYBACK; } static inline int snd_pcm_substream_is_playback_(const struct snd_pcm_substream *substream) { return snd_pcm_stream_is_playback(substream->stream); } static inline int snd_pcm_substream_is_playback(struct snd_pcm_substream *substream) { return snd_pcm_stream_is_playback(substream->stream); } #define snd_pcm_is_playback(x) _Generic((x), \ const int : snd_pcm_stream_is_playback_, \ int : snd_pcm_stream_is_playback, \ const unsigned int : snd_pcm_stream_is_playback_u, \ unsigned int : snd_pcm_stream_is_playbacku, \ const struct snd_pcm_substream * : snd_pcm_substream_is_playback_, \ struct snd_pcm_substream * : snd_pcm_substream_is_playback)(x) And I'm not sure how to handle "bit field" by _Generic. ${LINUX}/sound/pci/ac97/ac97_pcm.c:153:13: note: in expansion of macro 'snd_pcm_is_playback' 153 | if (snd_pcm_is_playback(pcm->stream)) | ^~~~~~~~~~~~~~~~~~~ ${LINUX}/sound/pci/ac97/ac97_pcm.c: In function 'snd_ac97_pcm_assign': ${LINUX}/include/sound/pcm.h:544:41: error: '_Generic' selector of type 'unsigned char:1' is not compatible with any association 544 | #define snd_pcm_is_playback(x) _Generic((x), \ Not using _Generic() is easy, "const int" version can handle everything (= "int", "const int", "unsigned int", "const unsigned int", "short", "const short", "bit field", etc). If there is better _Generic() grammar, I can follow it. If there wasn't, unfortunately let's give up this conversion... Thank you for your help !! Best regards --- Kuninori Morimoto
On Mon, 22 Jul 2024 07:58:41 +0200, Kuninori Morimoto wrote: > > > Hi Amadeusz, Takashi > > > > Perhaps you could use generics here, so you could have one caller for > > > both cases? > > > > > > Something like: > > > #define snd_pcm_is_playback(x) _Generic((x), \ > > > int : (x == SNDRV_PCM_STREAM_PLAYBACK), \ > > > struct snd_pcm_substream *substream * : (x->stream == > > > SNDRV_PCM_STREAM_PLAYBACK))(x) > > > or just call the above functions in it? > > Hmm... I couldn't compile above inline style. > I need to create function, and use it on _Generic(). > > And then, _Generic() is very picky for variable sytle. > This means I need to create function for "int" / "const int", > "unsigned int", "const unsigned int" > > static inline int snd_pcm_stream_is_playback_(const int stream) > { > return stream == SNDRV_PCM_STREAM_PLAYBACK; > } > > static inline int snd_pcm_stream_is_playback(int stream) > { > return stream == SNDRV_PCM_STREAM_PLAYBACK; > } > > static inline int snd_pcm_stream_is_playback_u(const unsigned int stream) > { > return stream == SNDRV_PCM_STREAM_PLAYBACK; > } > > static inline int snd_pcm_stream_is_playbacku(unsigned int stream) > { > return stream == SNDRV_PCM_STREAM_PLAYBACK; > } I really don't see any merit of those inline. If this simplifies the code, I'd agree, but that's adding just more code... > > static inline int snd_pcm_substream_is_playback_(const struct snd_pcm_substream *substream) > { > return snd_pcm_stream_is_playback(substream->stream); > } > > static inline int snd_pcm_substream_is_playback(struct snd_pcm_substream *substream) > { > return snd_pcm_stream_is_playback(substream->stream); > } > > #define snd_pcm_is_playback(x) _Generic((x), \ > const int : snd_pcm_stream_is_playback_, \ > int : snd_pcm_stream_is_playback, \ > const unsigned int : snd_pcm_stream_is_playback_u, \ > unsigned int : snd_pcm_stream_is_playbacku, \ > const struct snd_pcm_substream * : snd_pcm_substream_is_playback_, \ > struct snd_pcm_substream * : snd_pcm_substream_is_playback)(x) > > And I'm not sure how to handle "bit field" by _Generic. > > ${LINUX}/sound/pci/ac97/ac97_pcm.c:153:13: note: in expansion of macro 'snd_pcm_is_playback' > 153 | if (snd_pcm_is_playback(pcm->stream)) > | ^~~~~~~~~~~~~~~~~~~ > ${LINUX}/sound/pci/ac97/ac97_pcm.c: In function 'snd_ac97_pcm_assign': > ${LINUX}/include/sound/pcm.h:544:41: error: '_Generic' selector of type 'unsigned char:1' is not compatible with any association > 544 | #define snd_pcm_is_playback(x) _Generic((x), \ > > Not using _Generic() is easy, "const int" version can handle everything > (= "int", "const int", "unsigned int", "const unsigned int", "short", > "const short", "bit field", etc). > > If there is better _Generic() grammar, I can follow it. > If there wasn't, unfortunately let's give up this conversion... IMO, if the retrieval of the stream direction and its evaluation are done separately, there is little use of the inline functions like the above. It doesn't give a better readability nor code safety. That said, as of now, I'm not much convinced to go further. OTOH, I'm not strongly against taking this kind of change -- if other people do find it absolutely better, I'm ready to be convinced :) thanks, Takashi
On 7/22/24 10:16, Takashi Iwai wrote: > On Mon, 22 Jul 2024 07:58:41 +0200, > Kuninori Morimoto wrote: >> >> >> Hi Amadeusz, Takashi >> >>>> Perhaps you could use generics here, so you could have one caller for >>>> both cases? >>>> >>>> Something like: >>>> #define snd_pcm_is_playback(x) _Generic((x), \ >>>> int : (x == SNDRV_PCM_STREAM_PLAYBACK), \ >>>> struct snd_pcm_substream *substream * : (x->stream == >>>> SNDRV_PCM_STREAM_PLAYBACK))(x) >>>> or just call the above functions in it? >> >> Hmm... I couldn't compile above inline style. >> I need to create function, and use it on _Generic(). >> >> And then, _Generic() is very picky for variable sytle. >> This means I need to create function for "int" / "const int", >> "unsigned int", "const unsigned int" >> >> static inline int snd_pcm_stream_is_playback_(const int stream) >> { >> return stream == SNDRV_PCM_STREAM_PLAYBACK; >> } >> >> static inline int snd_pcm_stream_is_playback(int stream) >> { >> return stream == SNDRV_PCM_STREAM_PLAYBACK; >> } >> >> static inline int snd_pcm_stream_is_playback_u(const unsigned int stream) >> { >> return stream == SNDRV_PCM_STREAM_PLAYBACK; >> } >> >> static inline int snd_pcm_stream_is_playbacku(unsigned int stream) >> { >> return stream == SNDRV_PCM_STREAM_PLAYBACK; >> } > > I really don't see any merit of those inline. If this simplifies the > code, I'd agree, but that's adding just more code... > >> >> static inline int snd_pcm_substream_is_playback_(const struct snd_pcm_substream *substream) >> { >> return snd_pcm_stream_is_playback(substream->stream); >> } >> >> static inline int snd_pcm_substream_is_playback(struct snd_pcm_substream *substream) >> { >> return snd_pcm_stream_is_playback(substream->stream); >> } >> >> #define snd_pcm_is_playback(x) _Generic((x), \ >> const int : snd_pcm_stream_is_playback_, \ >> int : snd_pcm_stream_is_playback, \ >> const unsigned int : snd_pcm_stream_is_playback_u, \ >> unsigned int : snd_pcm_stream_is_playbacku, \ >> const struct snd_pcm_substream * : snd_pcm_substream_is_playback_, \ >> struct snd_pcm_substream * : snd_pcm_substream_is_playback)(x) >> >> And I'm not sure how to handle "bit field" by _Generic. >> >> ${LINUX}/sound/pci/ac97/ac97_pcm.c:153:13: note: in expansion of macro 'snd_pcm_is_playback' >> 153 | if (snd_pcm_is_playback(pcm->stream)) >> | ^~~~~~~~~~~~~~~~~~~ >> ${LINUX}/sound/pci/ac97/ac97_pcm.c: In function 'snd_ac97_pcm_assign': >> ${LINUX}/include/sound/pcm.h:544:41: error: '_Generic' selector of type 'unsigned char:1' is not compatible with any association >> 544 | #define snd_pcm_is_playback(x) _Generic((x), \ >> >> Not using _Generic() is easy, "const int" version can handle everything >> (= "int", "const int", "unsigned int", "const unsigned int", "short", >> "const short", "bit field", etc). >> >> If there is better _Generic() grammar, I can follow it. >> If there wasn't, unfortunately let's give up this conversion... > > IMO, if the retrieval of the stream direction and its evaluation are > done separately, there is little use of the inline functions like the > above. It doesn't give a better readability nor code safety. > > That said, as of now, I'm not much convinced to go further. > OTOH, I'm not strongly against taking this kind of change -- if other > people do find it absolutely better, I'm ready to be convinced :) The main issue I have with this patch is the continued confusion in variable naming between 'stream' and 'direction'. It's problematic on multiple platforms where a stream is typically identified by a DMA channel or ID of some sort. There's also the SoundWire 'stream' which has nothing to do with PCM devices. In the end people end-up drowning in too many 'streams', no one knows if the code refers to the data flow or the data direction.
On 7/22/2024 7:58 AM, Kuninori Morimoto wrote: > > Hi Amadeusz, Takashi > >>> Perhaps you could use generics here, so you could have one caller for >>> both cases? >>> >>> Something like: >>> #define snd_pcm_is_playback(x) _Generic((x), \ >>> int : (x == SNDRV_PCM_STREAM_PLAYBACK), \ >>> struct snd_pcm_substream *substream * : (x->stream == >>> SNDRV_PCM_STREAM_PLAYBACK))(x) >>> or just call the above functions in it? > > Hmm... I couldn't compile above inline style. > I need to create function, and use it on _Generic(). > > And then, _Generic() is very picky for variable sytle. > This means I need to create function for "int" / "const int", > "unsigned int", "const unsigned int" > > static inline int snd_pcm_stream_is_playback_(const int stream) > { > return stream == SNDRV_PCM_STREAM_PLAYBACK; > } > > static inline int snd_pcm_stream_is_playback(int stream) > { > return stream == SNDRV_PCM_STREAM_PLAYBACK; > } > > static inline int snd_pcm_stream_is_playback_u(const unsigned int stream) > { > return stream == SNDRV_PCM_STREAM_PLAYBACK; > } > > static inline int snd_pcm_stream_is_playbacku(unsigned int stream) > { > return stream == SNDRV_PCM_STREAM_PLAYBACK; > } > > static inline int snd_pcm_substream_is_playback_(const struct snd_pcm_substream *substream) > { > return snd_pcm_stream_is_playback(substream->stream); > } > > static inline int snd_pcm_substream_is_playback(struct snd_pcm_substream *substream) > { > return snd_pcm_stream_is_playback(substream->stream); > } > > #define snd_pcm_is_playback(x) _Generic((x), \ > const int : snd_pcm_stream_is_playback_, \ > int : snd_pcm_stream_is_playback, \ > const unsigned int : snd_pcm_stream_is_playback_u, \ > unsigned int : snd_pcm_stream_is_playbacku, \ > const struct snd_pcm_substream * : snd_pcm_substream_is_playback_, \ > struct snd_pcm_substream * : snd_pcm_substream_is_playback)(x) > > And I'm not sure how to handle "bit field" by _Generic. > > ${LINUX}/sound/pci/ac97/ac97_pcm.c:153:13: note: in expansion of macro 'snd_pcm_is_playback' > 153 | if (snd_pcm_is_playback(pcm->stream)) > | ^~~~~~~~~~~~~~~~~~~ > ${LINUX}/sound/pci/ac97/ac97_pcm.c: In function 'snd_ac97_pcm_assign': > ${LINUX}/include/sound/pcm.h:544:41: error: '_Generic' selector of type 'unsigned char:1' is not compatible with any association > 544 | #define snd_pcm_is_playback(x) _Generic((x), \ > > Not using _Generic() is easy, "const int" version can handle everything > (= "int", "const int", "unsigned int", "const unsigned int", "short", > "const short", "bit field", etc). > > If there is better _Generic() grammar, I can follow it. > If there wasn't, unfortunately let's give up this conversion... > > Thank you for your help !! > My mistake in example, I've used function syntax, notice (x) at the end, if you remove it, it seems to build without need to call inline functions: diff --git a/include/sound/pcm.h b/include/sound/pcm.h index 210096f124eed..e914fea59445e 100644 --- a/include/sound/pcm.h +++ b/include/sound/pcm.h @@ -496,6 +496,10 @@ struct snd_pcm_substream { #define SUBSTREAM_BUSY(substream) ((substream)->ref_count > 0) +#define snd_pcm_is_playback(x) _Generic((x), \ + int : (x == SNDRV_PCM_STREAM_PLAYBACK), \ + struct snd_pcm_substream * : (x->stream == SNDRV_PCM_STREAM_PLAYBACK)) + struct snd_pcm_str { int stream; /* stream (direction) */ diff --git a/sound/soc/intel/avs/pcm.c b/sound/soc/intel/avs/pcm.c index 68aa8de2b0c4e..7e9f0ac6a5bc6 100644 --- a/sound/soc/intel/avs/pcm.c +++ b/sound/soc/intel/avs/pcm.c @@ -351,7 +351,7 @@ static int avs_dai_hda_be_hw_free(struct snd_pcm_substream *substream, struct sn data->path = NULL; /* clear link <-> stream mapping */ - if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) + if (snd_pcm_is_playback(substream)) snd_hdac_ext_bus_link_clear_stream_id(data->link, hdac_stream(link_stream)->stream_tag); @@ -383,7 +383,7 @@ static int avs_dai_hda_be_prepare(struct snd_pcm_substream *substream, struct sn snd_hdac_ext_stream_reset(link_stream); snd_hdac_ext_stream_setup(link_stream, format_val); - if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) + if (snd_pcm_is_playback(substream)) snd_hdac_ext_bus_link_set_stream_id(data->link, hdac_stream(link_stream)->stream_tag); I've test compiled the above fine. As for ac97, seems like _Generic is impossible on bitfields, so perhaps just move it out of bitfield, to int? Thanks, Amadeusz
On 7/22/2024 10:47 AM, Pierre-Louis Bossart wrote: > The main issue I have with this patch is the continued confusion in > variable naming between 'stream' and 'direction'. It's problematic on > multiple platforms where a stream is typically identified by a DMA > channel or ID of some sort. There's also the SoundWire 'stream' which > has nothing to do with PCM devices. In the end people end-up drowning in > too many 'streams', no one knows if the code refers to the data flow or > the data direction. Oh yes, so I'm not the only one :D, I also would very much prefer if there was 'direction' instead of 'stream'.
Hi Amadeusz Thank you for your help > My mistake in example, I've used function syntax, notice (x) at the end, > if you remove it, it seems to build without need to call inline functions: Thanks. I was aware of that. Your example is calling snd_pcm_is_playback() with "snd_pcm_substream" only. It works well indeed. But I will get error if I call it with "int", like below. I don't know how to solve this issue and/or what does it mean... ${LINUX}/sound/soc/intel/avs/pcm.c: In function 'avs_dai_hda_be_prepare': ${LINUX}/include/sound/pcm.h:506:40: error: invalid type argument of '->' (have 'int') 506 | struct snd_pcm_substream * : ((x)->stream == SNDRV_PCM_STREAM_PLAYBACK)) | ^~ ${LINUX}/sound/soc/intel/avs/pcm.c:375:13: note: in expansion of macro 'snd_pcm_is_playback' 375 | if (snd_pcm_is_playback(substream->stream)) | ^~~~~~~~~~~~~~~~~~~ Below is the code. It is copied your example, and I updated it to use both "int" and "snd_pcm_substream". - if (snd_pcm_is_playback(substream)) + if (snd_pcm_is_playback(substream->stream)) ---------------- diff --git a/include/sound/pcm.h b/include/sound/pcm.h index 3edd7a7346da..a4916342f715 100644 --- a/include/sound/pcm.h +++ b/include/sound/pcm.h @@ -501,6 +501,9 @@ struct snd_pcm_substream { #define SUBSTREAM_BUSY(substream) ((substream)->ref_count > 0) +#define snd_pcm_is_playback(x) _Generic((x), \ + int : ((x) == SNDRV_PCM_STREAM_PLAYBACK), \ + struct snd_pcm_substream * : ((x)->stream == SNDRV_PCM_STREAM_PLAYBACK)) struct snd_pcm_str { int stream; /* stream (direction) */ diff --git a/sound/soc/intel/avs/pcm.c b/sound/soc/intel/avs/pcm.c index c76b86254a8b..79ae6a5df9c2 100644 --- a/sound/soc/intel/avs/pcm.c +++ b/sound/soc/intel/avs/pcm.c @@ -331,7 +331,7 @@ static int avs_dai_hda_be_hw_free(struct snd_pcm_substream *substream, struct sn if (!link) return -EINVAL; - if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) + if (snd_pcm_is_playback(substream)) snd_hdac_ext_bus_link_clear_stream_id(link, hdac_stream(link_stream)->stream_tag); return 0; @@ -372,7 +372,7 @@ static int avs_dai_hda_be_prepare(struct snd_pcm_substream *substream, struct sn if (!link) return -EINVAL; - if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) + if (snd_pcm_is_playback(substream->stream)) snd_hdac_ext_bus_link_set_stream_id(link, hdac_stream(link_stream)->stream_tag); ret = avs_dai_prepare(substream, dai); ---------------- Thank you for your help !! Best regards --- Kuninori Morimoto
Hi Amadeusz, Pierre-Louis, Takashi Thank you for your helps > That said, as of now, I'm not much convinced to go further. > OTOH, I'm not strongly against taking this kind of change -- if other > people do find it absolutely better, I'm ready to be convinced :) (snip) > > The main issue I have with this patch is the continued confusion in > > variable naming between 'stream' and 'direction'. It's problematic on > > multiple platforms where a stream is typically identified by a DMA > > channel or ID of some sort. There's also the SoundWire 'stream' which > > has nothing to do with PCM devices. In the end people end-up drowning in > > too many 'streams', no one knows if the code refers to the data flow or > > the data direction. (snip) > Oh yes, so I'm not the only one :D, I also would very much prefer if > there was 'direction' instead of 'stream'. For now, unfortunately, using _Generic() makes code more complex, because many drivers are using own direction variables (with unsigned, const, short, bitfield, etc), and _Generic() need to know it. Not _Generic() code is not convenience, but not so bad (?). If so, can below acceptable ? snd_pcm_direction_is_playback(const int direction); snd_pcm_direction_is_capture(const int direction); snd_pcm_substream_is_playback(const struct snd_pcm_substream *); snd_pcm_substream_is_capture(const struct snd_pcm_substream *); ... BTW, I noticed some drivers are using below, is there any difference ?? substream->pstr->stream substream->stream Thank you for your help !! Best regards --- Kuninori Morimoto
diff --git a/include/sound/pcm.h b/include/sound/pcm.h index 3edd7a7346daa..024dc2b337154 100644 --- a/include/sound/pcm.h +++ b/include/sound/pcm.h @@ -501,6 +501,25 @@ struct snd_pcm_substream { #define SUBSTREAM_BUSY(substream) ((substream)->ref_count > 0) +static inline int snd_stream_is_playback(int stream) +{ + return stream == SNDRV_PCM_STREAM_PLAYBACK; +} + +static inline int snd_stream_is_capture(int stream) +{ + return stream == SNDRV_PCM_STREAM_CAPTURE; +} + +static inline int snd_substream_is_playback(const struct snd_pcm_substream *substream) +{ + return snd_stream_is_playback(substream->stream); +} + +static inline int snd_substream_is_capture(const struct snd_pcm_substream *substream) +{ + return snd_stream_is_capture(substream->stream); +} struct snd_pcm_str { int stream; /* stream (direction) */
Many drivers are using below code to know the direction. if (direction == SNDRV_PCM_STREAM_PLAYBACK) Add snd_stream_is_playback/capture() macro to handle it. It also adds snd_substream_is_playback/capture() to handle snd_pcm_substream. Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> --- include/sound/pcm.h | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+)