Re: [tyndur-devel] [PATCH] CDI/Sound

[Kevin Wolf <kevin@xxxxxxxxxx>]

On Tue, Aug 11, 2009 at 01:22:15AM +0200, Patrick@xxxxxxxxxxxxxxxxxxxx wrote:
> From: deathly-sequences@xxxxxx <snake707@Cazad-Dum.(none)>
> 
> ---
>  src/modules/cdi/ac97/Makefile.all    |    6 +
>  src/modules/cdi/ac97/ac97.c          |  189 ++++++++++++++++++++++++++++++++++
>  src/modules/cdi/ac97/includes/ac97.h |   80 ++++++++++++++
>  src/modules/cdi/ac97/main.c          |  123 ++++++++++++++++++++++
>  src/modules/cdi/include/cdi.h        |    1 +
>  src/modules/cdi/include/cdi/sound.h  |  151 +++++++++++++++++++++++++++
>  6 files changed, 550 insertions(+), 0 deletions(-)
>  create mode 100644 src/modules/cdi/ac97/Makefile.all
>  create mode 100644 src/modules/cdi/ac97/ac97.c
>  create mode 100644 src/modules/cdi/ac97/includes/ac97.h
>  create mode 100644 src/modules/cdi/ac97/main.c
>  create mode 100644 src/modules/cdi/include/cdi/sound.h

Ich ignoriere mal den AC97-Treiber, von dem hast du ja gesagt, dass er
noch nicht fertig genug ist. Sag Bescheid, wenn ich ihn anschauen soll.

> diff --git a/src/modules/cdi/include/cdi.h b/src/modules/cdi/include/cdi.h
> index c6215a6..c3a48ee 100644
> --- a/src/modules/cdi/include/cdi.h
> +++ b/src/modules/cdi/include/cdi.h
> @@ -25,6 +25,7 @@ typedef enum {
>      CDI_NETWORK         = 1,
>      CDI_STORAGE         = 2,
>      CDI_SCSI            = 3,
> +    CDI_SOUND           = 4,
>  } cdi_device_type_t;
>  
>  struct cdi_driver;
> diff --git a/src/modules/cdi/include/cdi/sound.h b/src/modules/cdi/include/cdi/sound.h
> new file mode 100644
> index 0000000..680553a
> --- /dev/null
> +++ b/src/modules/cdi/include/cdi/sound.h
> @@ -0,0 +1,151 @@
> +
> +/** @file sound.h 
> + *
> + * Diese Datei enthält die Datenstrukturen, von denen die Datenstrukturen der
> + * Treiber abgeleitet werden. Außerdem werden hier die Funktionsprototypen für
> + * die Schnittstelle Soundtreiber, CDI definiert.
> + **/
> +
> +/**
> + * Treiber für die Soundkarten.
> + *
> + * @defgroup sound
> + **/
> +
> +#ifndef __sound_h__
> +#define __sound_h__
> +
> +#include <stdint.h>
> +#include <stddef.h>
> +
> +#include "cdi.h"
> +
> +/* Angelehnt an net.h */
> +
> +struct cdi_sound_device{
> +    struct cdi_device dev;
> +    /* Im folgenden bezeichnet driver immer das Gerät. */

Das klingt irgendwie auffällig falschrum. Ein Gerät sollte ein
cdi_sound_device sein. Mit einem cdi_sound_driver kann der Treiber
vermutlich nicht so viel anfangen, wenn er die ganzen Informationen
ordentlich in einem cdi_sound_device abgelegt hat (wie das dein
AC97-Treiber zu tun scheint)

> +    /* Ausführlichere Kommentare/Dokumentation findet man in den jeweiligen
> +       Implementationen der Treiber. Hier werden nur Funktionszeiger
> +       definiert. */
> +    
> +    /** @brief Damit das Gerät auch beginnt Daten abzuspielen. 
> +     *  
> +     *  Weißt den Treiber an, Daten abzuspielen, wenn neue vorhanden sind.
> +     *
> +     * @param driver Treiber, der die Daten abspielen soll.
> +     *
> +     */
> +    void (*cdi_sound_play)(struct cdi_sound_driver *driver);
> +    
> +    /** @brief Hält die Audioausgabe des Treibers an.
> +     *
> +     * Wenn der Treiber nicht gerade von einer anderen Anwendung blockiert wird,
> +     * wird die Audioausgabe angehalten. Wird ignoriert, wenn der Treiber
> +     * 'in use' ist.
> +     *
> +     * @param driver Treiber, der die Audioausgabe anhalten soll.
> +     */
> +    void (*cdi_sound_stop)(struct cdi_sound_driver *driver);

Ich überlege grad, ob das reicht. Irgendeine Lib dahinter wird ja
vermutlich mehrere Sounds zusammenmixen. Wenn jetzt einer von zwei
gerade abgespielten Sounds gestoppt wird, brauchen wir eigentlich keinen
kompletten Stop, sondern...

* ... ein Stop direkt gefolgt von einem Start, das den übriggebliebenen
  Sound wieder startet. Dazu brauchen wir wenigstens die aktuelle
  Position, an der angehalten wurde.
* ... oder ein cdi_sound_replace() oder sowas, das einen neuen Puffer
   kriegt, dort die aktuelle Stelle raussucht und an die Soundkarte
   schickt, was jetzt zu spielen ist

Ist der Gedanke so ungefähr nachvollziehbar?

> +    

> +    /** @brief Ein Prozess darf die in_use Option betätigen.
> +     *
> +     * Die in use Option ist mir eingefallen, als ich mir das Zusammenspiel
> +     * Amarok, Firefox mit flashplayer plugin und der rest des Systems angesehen
> +     * habe. Die schließen sich gegenseitig aus. Sobald Amarok an ist, kann
> +     * der Rest nichts mehr abspielen. Hat auch gute Seiten. Funktionszeiger ...
> +     *
> +     * @param driver Treiber der in use gestellt werden soll.
> +     * @param pid Prozess ID des Prozesses. Dient später nur zur Überprüfung.
> +     *
> +     * @return 0 bei Erfolg. 1 bei Misserfolg.
> +     *
> +     */
> +    uint32_t (*cdi_sound_in_use_enable)(struct cdi_sound_driver *driver,
> +                                    uint32_t pid);
> +    

Zum einen solltest du drauf achten, keine Leerzeichen am Zeilenende zu
haben (gibt noch einige mehr davon, hier ist es mir aufgefallen).

Zum anderen ist das hier die falsche Ebene. Den Soundtreiber haben
Sachen wie Prozesse nicht zu interessieren. Der Treiber spielt Sound ab,
und das war's. Um Prozesse kann sich dann vielleicht die CDI-Lib
kümmern.

> +    /** @brief Der gleiche Prozess darf das in_use Privileg abgeben.
> +     *
> +     * Anhand von pid wird nur überprüft ob der richtige Prozess die in use
> +     * Option abstellt. Funktionszeiger ...
> +     * 
> +     * @param driver Treiber der die in use Option abschalten soll.
> +     * @param pid Prozessnummer.
> +     *
> +     * @return 0 bei Erfolg. 1 bei Misserfolg.
> +     */
> +    uint32_t (*cdi_sound_in_use_disable)(struct cdi_sound_driver *driver,
> +                                     uint32_t pid);
> +    
> +    /** @brief Funktionszeiger für eine Schnittstelle, in der man den aktuellen
> +     *          Status des Treibers/Gerätes abrufen kann.
> +     *
> +     * Diese Funktion ist dafür gedacht um herauszufinden ob der Treiber gerade
> +     * Daten abspielt oder nicht. Hier ist die Schnittstelle Treiber/CDI
> +     * 
> +     * @param driver Soundtreiber, dessen Zustand ermittelt werden soll.
> +     *
> +     * @return Die Returncodes können von Treiber zu Treiber variieren. Hier
> +     *         ist eine Liste, von Codes, die auf jeden Fall genauso
> +     *         implementiert werden sollen (Zusätzliche können angehangen
> +     *         werden): 0 - Treiber/Gerät nicht arbeitsbereit/initialisiert.
> +     *         1 - Gerät arbeitsbereit. Spielt keine Daten ab. 2 - Treiber
> +     *         erhält Daten. 4 - Gerät spielt Daten ab. Datenempfang möglich.
> +     *         8 - Gerät ist gestoppt, aber es sind noch Daten zum spielen
> +     *         übrig. Neue Daten können jederzeit hinzugefügt werden.
> +     *         16 - Bit ist gesetzt, wenn Qucik Mixing (lineare Superposition
> +     *         mit Clippingunterdrückung) erlaubt ist. 32 - Dieses Bit zeigt an
> +     *         ob der Treiber gerade von einem Prozess "blockiert" wird.
> +     *       
> +     */
> +    uint32_t (*cdi_sound_driver_state)(struct cdi_sound_driver *driver);

Nein, sie kann nicht von Treiber zu Treiber variieren. Der Sinn und
Zweck von CDI ist ein _standardisiertes_ Interface. Also: Ich schreibe
anhand dieser Dokumentation eine CDI-Implementierung für mein OS, dann
nehme ich einen beliebigen dazu passenden Soundtreiber und es
funktioniert.

Oder ich schreibe einen Soundtreiber, der sich daran hält, und kopiere
ihn auf jedes beliebige OS, das CDI kann, und er funktioniert.

An dieser Stelle braucht es also eine definitive Aussage, was es gibt
und was nicht. Die Sachen, die es gibt, sollten als Konstante definiert
werden (z.B. irgendwo ein enum für die zulässigen status) statt direkt
magische Zahlen zu verwenden.

> +    
> +    /** @brief Funktionszeiger für eine Schnittstelle, die dem Treiber die
> +     *          als nächstes abzuspielenden Daten übergibt.
> +     *
> +     * Dies ist ein Funktionszeiger und für die Schnittstelle Treiber CDI
> +     * gedacht.
> +     *
> +     * @param driver Soundtreiber an den die Daten geschickt werden sollen.
> +     * @param data_start Zeiger auf den Beginn der Daten. Es wird angenommen,
> +     *                   dass alle Daten hintereinander liegen.
> +     * @param data_length Länge des Datenblockes.
> +     * @param pid Datenbereiche werden Treiberintern mit einer PID versehen, um
> +     *            diese z.B. noch zurückziehen zu können. Prozess beendet.
> +     *            Datenbereiche wurden deallokiert.
> +     *
> +     * @return 0 - bei Erfolg, 1 - bei Fehler, wiederhohlung kann ihn beheben
> +     *         2 - Fehler nicht durch wiederhohlte Ausführung behebbar
> +     *         kann. Wenn Gerät z.B. von einem Prozess benutzt wird, der Mixing
> +     *         nicht zulassen will. Ansonnsten, werden Daten von verschiednen
> +     *         Geräten linear superponiert, oder einfach nacheinander
> +     *         abgespielt (Flags beachten).
> +     */
> +    uint32_t (*cdi_sound_input_data)(struct cdi_sound_driver *driver,
> +                                     uint32_t* data_start,
> +                                     uint32_t data_length,
> +                                     uint32_t pid);

Siehe oben, pid geht den Treiber nichts an.

Kevin