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

[Paul Kocialkowski <paul.kocialkowski@linux.intel.com>]

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@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.

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?

-- 
Paul Kocialkowski <paul.kocialkowski@linux.intel.com>
Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo, Finland
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx