Re: [PATCH] ASoC: intel: skylake: Drop superfluous mmap callback

[Takashi Iwai <tiwai@suse.de>]

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