[Intel-gfx] [PATCH i-g-t 2/3] lib: Add ALSA library with dedicated helpers

[Arkadiusz Hiler]

On Mon, Aug 21, 2017 at 12:39:58PM +0300, Paul Kocialkowski wrote:
> On Mon, 2017-08-21 at 10:25 +0300, Arkadiusz Hiler wrote:
> > On Fri, Aug 18, 2017 at 12:46:38PM +0300, Arkadiusz Hiler wrote:
> > > On Thu, Aug 17, 2017 at 07:05:56PM +0300, Paul Kocialkowski wrote:
> > > > This introduces an ALSA library, with dedicated helpers for
> > > > handling
> > > > playback and capture. It handles ALSA device identification and
> > > > configuration as well as a run loop with callback mechanisms for
> > > > feeding
> > > > output data and handling input data.
> > > > 
> > > > This library paves the way for testing audio going through display
> > > > connectors, such as HDMI.
> > > > 
> > > > Signed-off-by: Paul Kocialkowski <paul.kocialkowski at linux.intel.co
> > > > m>
> > > > ---
> > > >  configure.ac                                       |   3 +
> > > >  .../intel-gpu-tools/intel-gpu-tools-docs.xml       |   1 +
> > > >  lib/Makefile.am                                    |   7 +
> > > >  lib/igt.h                                          |   1 +
> > > >  lib/igt_alsa.c                                     | 624
> > > > +++++++++++++++++++++
> > > >  lib/igt_alsa.h                                     |  60 ++
> > > >  6 files changed, 696 insertions(+)
> > > >  create mode 100644 lib/igt_alsa.c
> > > >  create mode 100644 lib/igt_alsa.h
> > > > 
> > > > diff --git a/configure.ac b/configure.ac
> > > > index 50aa86b5..e66273a4 100644
> > > > --- a/configure.ac
> > > > +++ b/configure.ac
> > > > @@ -219,6 +219,9 @@ if test "x$enable_chamelium" = xyes; then
> > > >  	AC_DEFINE(HAVE_CHAMELIUM, 1, [Enable Chamelium support])
> > > >  fi
> > > >  
> > > > +PKG_CHECK_MODULES(ALSA, [alsa], [alsa=yes], [alsa=no])
> > > > +AM_CONDITIONAL(HAVE_ALSA, [test "x$alsa" = xyes])
> > > 
> > > please mention the new dependency in the README
> > 
> > One more thing that came to my mind - how do we want to have this new
> > dependency handled configure-time?
> > 
> > I see three options:
> > 
> > 1. Having it as optional component, so after we --enable-chamelium we
> > can
> > end up with the alsa part either being there or not.
> > 
> > 2. Having to explicitly enable it, so that only with --enable-alsa we
> > will have this new, shiny feature (and a nice ./configure-time fail).
> > 
> > 3. Having to explicitly disable it, i.e. --enable-chamelium enables
> > ALSA
> > check, if it's not there configure fails and we can explicitly disable
> > it, resulting in --enable-chamelium --disable-alsa switch combo.
> > 
> > As a rule of thumb, anything similar to option #1 should be avoided to
> > not suprise people with missing features depending on machine they
> > compile on - unless we collectively decide that having optional
> > component is okay (e.g. in case we have a fallback so it doesn't
> > matter
> > that much).
> > 
> > IMO in this case we should go with option #2.
> 
> Note that this series is not tied to the chamelium at all: it requires
> an HDMI-VGA adapter with audio out instead of a chamelium.
> 
> For this reason, it wouldn't make sense to tie it to --enable-chamelium.

Ow, right. I've missed that. Thanks for clarification!

> The problem remains the same though: the audio part (patch 1/3) requires
> the GSL library and the alsa part requires the alsa lib and we need to
> decide how to handle the dependencies.
> 
> I think what would make the most sense is to have an --enable-audio
> parameter (referring to the newly-introduced audio test, not the audio
> library) that requires the alsa and gsl libraries. Then, when audio
> support is added to the chamelium code, the alsa library should become a
> dependency for --enable-chamelium as well.
> 
> For the latter, it would be consistent with the idea that --enable-
> chamelium pulls-in all possible chamelium tests and thus requires all
> their dependencies, instead of only enabling the tests for which
> dependencies are installed.
> 
> Does that seem agreeable?

LGTM, carry on! :-)