| Message ID | 20210728141930.17740-1-tiwai@suse.de (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 9398a834700e124027e73874450e6aa35fae479e |
| Headers | show |
| Series | ASoC: intel: skylake: Drop superfluous mmap callback | expand |
On 2021-07-28 4:19 PM, Takashi Iwai wrote: > skl_platform_soc_mmap() just calls the standard mmap helper, hence > it's superfluous. Let's drop it. > > Signed-off-by: Takashi Iwai <tiwai@suse.de> > --- > sound/soc/intel/skylake/skl-pcm.c | 8 -------- > 1 file changed, 8 deletions(-) > > diff --git a/sound/soc/intel/skylake/skl-pcm.c b/sound/soc/intel/skylake/skl-pcm.c > index b1ca64d2f7ea..c604200de80e 100644 > --- a/sound/soc/intel/skylake/skl-pcm.c > +++ b/sound/soc/intel/skylake/skl-pcm.c > @@ -1214,13 +1214,6 @@ static snd_pcm_uframes_t skl_platform_soc_pointer( > return bytes_to_frames(substream->runtime, pos); > } > > -static int skl_platform_soc_mmap(struct snd_soc_component *component, > - struct snd_pcm_substream *substream, > - struct vm_area_struct *area) > -{ > - return snd_pcm_lib_default_mmap(substream, area); > -} > - > static u64 skl_adjust_codec_delay(struct snd_pcm_substream *substream, > u64 nsec) > { > @@ -1460,7 +1453,6 @@ static const struct snd_soc_component_driver skl_component = { > .trigger = skl_platform_soc_trigger, > .pointer = skl_platform_soc_pointer, > .get_time_info = skl_platform_soc_get_time_info, > - .mmap = skl_platform_soc_mmap, > .pcm_construct = skl_platform_soc_new, > .module_get_upon_open = 1, /* increment refcount when a pcm is opened */ > }; > Thanks for the input, Takashi. While I welcome the change, have two quick questions: 1) Does this impact hw_support_mmap() and its user behavior? i.e. is there some implicit behavior change that skylake-users may experience along the way? 2) Since snd_pcm_mmap_data() defaults to snd_pcm_lib_default_mmap() why not update ops assignment - snd_pcm_set_ops() - in such a way that if/else is no longer needed in the former? Pretty sure other drivers have been updated in similar fashion and my two cents should not be blocking integration: Reviewed-by: Cezary Rojewski <cezary.rojewski@intel.com> Czarek
On Fri, 30 Jul 2021 15:59:54 +0200, Cezary Rojewski wrote: > > On 2021-07-28 4:19 PM, Takashi Iwai wrote: > > skl_platform_soc_mmap() just calls the standard mmap helper, hence > > it's superfluous. Let's drop it. > > > > Signed-off-by: Takashi Iwai <tiwai@suse.de> > > --- > > sound/soc/intel/skylake/skl-pcm.c | 8 -------- > > 1 file changed, 8 deletions(-) > > > > diff --git a/sound/soc/intel/skylake/skl-pcm.c b/sound/soc/intel/skylake/skl-pcm.c > > index b1ca64d2f7ea..c604200de80e 100644 > > --- a/sound/soc/intel/skylake/skl-pcm.c > > +++ b/sound/soc/intel/skylake/skl-pcm.c > > @@ -1214,13 +1214,6 @@ static snd_pcm_uframes_t skl_platform_soc_pointer( > > return bytes_to_frames(substream->runtime, pos); > > } > > -static int skl_platform_soc_mmap(struct snd_soc_component > > *component, > > - struct snd_pcm_substream *substream, > > - struct vm_area_struct *area) > > -{ > > - return snd_pcm_lib_default_mmap(substream, area); > > -} > > - > > static u64 skl_adjust_codec_delay(struct snd_pcm_substream *substream, > > u64 nsec) > > { > > @@ -1460,7 +1453,6 @@ static const struct snd_soc_component_driver skl_component = { > > .trigger = skl_platform_soc_trigger, > > .pointer = skl_platform_soc_pointer, > > .get_time_info = skl_platform_soc_get_time_info, > > - .mmap = skl_platform_soc_mmap, > > .pcm_construct = skl_platform_soc_new, > > .module_get_upon_open = 1, /* increment refcount when a pcm is opened */ > > }; > > > > Thanks for the input, Takashi. > While I welcome the change, have two quick questions: > > 1) Does this impact hw_support_mmap() and its user behavior? i.e. is > there some implicit behavior change that skylake-users may experience > along the way? hw_support_mmap() must return true for this case as long as you've set up the buffer in the right way (either preallocation or managed). > 2) Since snd_pcm_mmap_data() defaults to snd_pcm_lib_default_mmap() > why not update ops assignment - snd_pcm_set_ops() - in such a way that > if/else is no longer needed in the former? Simply put: when the buffer is allocated via snd_pcm_set_managed_buffer*(), you can omit the mmap callback. The only case you need the mmap callback is only when a special buffer is used or it needs a special way of mmap itself. > Pretty sure other drivers have been updated in similar fashion and my > two cents should not be blocking integration: > > Reviewed-by: Cezary Rojewski <cezary.rojewski@intel.com> Thanks! Takashi
On Wed, 28 Jul 2021 16:19:30 +0200, Takashi Iwai wrote: > skl_platform_soc_mmap() just calls the standard mmap helper, hence > it's superfluous. Let's drop it. Applied to https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next Thanks! [1/1] ASoC: intel: skylake: Drop superfluous mmap callback commit: 9398a834700e124027e73874450e6aa35fae479e All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted. You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed. If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced. Please add any relevant lists and maintainers to the CCs when replying to this mail. Thanks, Mark
On 2021-07-30 8:20 PM, Takashi Iwai wrote: > On Fri, 30 Jul 2021 15:59:54 +0200, > Cezary Rojewski wrote: ... >> >> Thanks for the input, Takashi. >> While I welcome the change, have two quick questions: >> >> 1) Does this impact hw_support_mmap() and its user behavior? i.e. is >> there some implicit behavior change that skylake-users may experience >> along the way? > > hw_support_mmap() must return true for this case as long as you've set > up the buffer in the right way (either preallocation or managed). hw_support_mmap() has an 'if' checking substream->ops->mmap. ASoC framework won't assign snd_soc_pcm_component_mmap as mmap-ops in soc_new_pcm() if component_driver didn't provided custom handler. This makes me believe function's behavior may change but I might as well be missing something here. >> 2) Since snd_pcm_mmap_data() defaults to snd_pcm_lib_default_mmap() >> why not update ops assignment - snd_pcm_set_ops() - in such a way that >> if/else is no longer needed in the former? > > Simply put: when the buffer is allocated via > snd_pcm_set_managed_buffer*(), you can omit the mmap callback. > The only case you need the mmap callback is only when a special buffer > is used or it needs a special way of mmap itself. Comment of my was more of a suggestion i.e. snd_pcm_mmap_data() has an if-statement: if (substream->ops->mmap) err = substream->ops->mmap(substream, area); else err = snd_pcm_lib-default_mmap(substream, area); I believe this could be replaced by one-liner with snd_pcm_set_ops() modified: do the validity check + default assignment there instead. Czarek
On Mon, 02 Aug 2021 10:07:12 +0200, Cezary Rojewski wrote: > > On 2021-07-30 8:20 PM, Takashi Iwai wrote: > > On Fri, 30 Jul 2021 15:59:54 +0200, > > Cezary Rojewski wrote: > > ... > > >> > >> Thanks for the input, Takashi. > >> While I welcome the change, have two quick questions: > >> > >> 1) Does this impact hw_support_mmap() and its user behavior? i.e. is > >> there some implicit behavior change that skylake-users may experience > >> along the way? > > > > hw_support_mmap() must return true for this case as long as you've set > > up the buffer in the right way (either preallocation or managed). > > hw_support_mmap() has an 'if' checking substream->ops->mmap. ASoC > framework won't assign snd_soc_pcm_component_mmap as mmap-ops in > soc_new_pcm() if component_driver didn't provided custom handler. > > This makes me believe function's behavior may change but I might as > well be missing something here. Yes, in that sense, the behavior of the driver has changed, but it's only about how the function gets called and nothing visible as the actual user-visible behavior. ASoC core leaves the PCM mmap callback empty, and ALSA core calls snd_pcm_lib_default_mmap() in the end, which is the same function that was called before the patch. > >> 2) Since snd_pcm_mmap_data() defaults to snd_pcm_lib_default_mmap() > >> why not update ops assignment - snd_pcm_set_ops() - in such a way that > >> if/else is no longer needed in the former? > > > > Simply put: when the buffer is allocated via > > snd_pcm_set_managed_buffer*(), you can omit the mmap callback. > > The only case you need the mmap callback is only when a special buffer > > is used or it needs a special way of mmap itself. > > Comment of my was more of a suggestion i.e. snd_pcm_mmap_data() has an > if-statement: > > if (substream->ops->mmap) > err = substream->ops->mmap(substream, area); > else > err = snd_pcm_lib-default_mmap(substream, area); > > I believe this could be replaced by one-liner with snd_pcm_set_ops() > modified: do the validity check + default assignment there instead. Well, at the point of snd_pcm_set_ops() called, it's not guaranteed that the driver has already set up the buffer. So we can't check at that moment, but at most, only at the point when the callback gets actually called -- and that's not easy, either. e.g. HD-audio driver calls snd_pcm_lib_default_mmap() from its own mmap callback as a fallback case where no special handling is needed. ... But, you comment made me taking a look back at the implementation of HD-audio mmap, and this brought an idea for a further cleanup; the pgprot setup could be rather in the common mmap code of WC pages, instead of the driver-specific one. Then we'll be able to get rid of the all calls of snd_pcm_lib_default_mmap(). thanks, Takashi
diff --git a/sound/soc/intel/skylake/skl-pcm.c b/sound/soc/intel/skylake/skl-pcm.c index b1ca64d2f7ea..c604200de80e 100644 --- a/sound/soc/intel/skylake/skl-pcm.c +++ b/sound/soc/intel/skylake/skl-pcm.c @@ -1214,13 +1214,6 @@ static snd_pcm_uframes_t skl_platform_soc_pointer( return bytes_to_frames(substream->runtime, pos); } -static int skl_platform_soc_mmap(struct snd_soc_component *component, - struct snd_pcm_substream *substream, - struct vm_area_struct *area) -{ - return snd_pcm_lib_default_mmap(substream, area); -} - static u64 skl_adjust_codec_delay(struct snd_pcm_substream *substream, u64 nsec) { @@ -1460,7 +1453,6 @@ static const struct snd_soc_component_driver skl_component = { .trigger = skl_platform_soc_trigger, .pointer = skl_platform_soc_pointer, .get_time_info = skl_platform_soc_get_time_info, - .mmap = skl_platform_soc_mmap, .pcm_construct = skl_platform_soc_new, .module_get_upon_open = 1, /* increment refcount when a pcm is opened */ };
skl_platform_soc_mmap() just calls the standard mmap helper, hence it's superfluous. Let's drop it. Signed-off-by: Takashi Iwai <tiwai@suse.de> --- sound/soc/intel/skylake/skl-pcm.c | 8 -------- 1 file changed, 8 deletions(-)