All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] ASoC: intel: skylake: Drop superfluous mmap callback
@ 2021-07-28 14:19 Takashi Iwai
  2021-07-30 13:59 ` Cezary Rojewski
  2021-07-30 19:03 ` Mark Brown
  0 siblings, 2 replies; 6+ messages in thread
From: Takashi Iwai @ 2021-07-28 14:19 UTC (permalink / raw)
  To: alsa-devel; +Cc: Cezary Rojewski, Mark Brown, Pierre-Louis Bossart

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 */
 };
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH] ASoC: intel: skylake: Drop superfluous mmap callback
  2021-07-28 14:19 [PATCH] ASoC: intel: skylake: Drop superfluous mmap callback Takashi Iwai
@ 2021-07-30 13:59 ` Cezary Rojewski
  2021-07-30 18:20   ` Takashi Iwai
  2021-07-30 19:03 ` Mark Brown
  1 sibling, 1 reply; 6+ messages in thread
From: Cezary Rojewski @ 2021-07-30 13:59 UTC (permalink / raw)
  To: Takashi Iwai, alsa-devel; +Cc: Mark Brown, Pierre-Louis Bossart

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] ASoC: intel: skylake: Drop superfluous mmap callback
  2021-07-30 13:59 ` Cezary Rojewski
@ 2021-07-30 18:20   ` Takashi Iwai
  2021-08-02  8:07     ` Cezary Rojewski
  0 siblings, 1 reply; 6+ messages in thread
From: Takashi Iwai @ 2021-07-30 18:20 UTC (permalink / raw)
  To: Cezary Rojewski; +Cc: alsa-devel, Mark Brown, Pierre-Louis Bossart

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] ASoC: intel: skylake: Drop superfluous mmap callback
  2021-07-28 14:19 [PATCH] ASoC: intel: skylake: Drop superfluous mmap callback Takashi Iwai
  2021-07-30 13:59 ` Cezary Rojewski
@ 2021-07-30 19:03 ` Mark Brown
  1 sibling, 0 replies; 6+ messages in thread
From: Mark Brown @ 2021-07-30 19:03 UTC (permalink / raw)
  To: Takashi Iwai, alsa-devel
  Cc: Cezary Rojewski, Mark Brown, Pierre-Louis Bossart

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] ASoC: intel: skylake: Drop superfluous mmap callback
  2021-07-30 18:20   ` Takashi Iwai
@ 2021-08-02  8:07     ` Cezary Rojewski
  2021-08-02  8:32       ` Takashi Iwai
  0 siblings, 1 reply; 6+ messages in thread
From: Cezary Rojewski @ 2021-08-02  8:07 UTC (permalink / raw)
  To: Takashi Iwai; +Cc: alsa-devel, Mark Brown, Pierre-Louis Bossart

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] ASoC: intel: skylake: Drop superfluous mmap callback
  2021-08-02  8:07     ` Cezary Rojewski
@ 2021-08-02  8:32       ` Takashi Iwai
  0 siblings, 0 replies; 6+ messages in thread
From: Takashi Iwai @ 2021-08-02  8:32 UTC (permalink / raw)
  To: Cezary Rojewski; +Cc: alsa-devel, Mark Brown, Pierre-Louis Bossart

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2021-08-02  8:33 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-28 14:19 [PATCH] ASoC: intel: skylake: Drop superfluous mmap callback Takashi Iwai
2021-07-30 13:59 ` Cezary Rojewski
2021-07-30 18:20   ` Takashi Iwai
2021-08-02  8:07     ` Cezary Rojewski
2021-08-02  8:32       ` Takashi Iwai
2021-07-30 19:03 ` Mark Brown

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.