add SoundBlaster output driver for DOS

[ccawley2011]

Opened a PR for this to avoid review comments getting lost when rebasing the branch.

[sezero]

@AliceLR: can you have a look whenever you are able?

[AliceLR]

Sorry for missing this. Comparing the two branches, the changes affecting DJGPP seem generally reasonable from my limited knowledge. The Watcom-specific parts are entirely over my head. I'll try to at least test this with DJGPP on my end soon.

For future reference... https://github.com/sezero/xmp-cli/compare/dos...ccawley2011:xmp-cli:dos-watcom

[sezero]

I had forgotten this one too.

One thing I may do is remove SB1 code and require SB2 and newer so that I can drop the interrupt stuff. which is the main source of complications.

[sezero]

The irq stuff in allegro is in asm, not in C code, so I am not 100% comfortable with the current solution.

The easiest solution seems to be removing support for sb versions older than sb2: sb2, sbpro, sb16 & compatibles are capable of auto- dma and don't need interrupt handling. Removing the irq stuff also meant removing detection of 'missing sound card parameters': we do rely on correctly installed driver.

Does this attached patch look correct? patch.txt

[AliceLR]

My only comment is that it might be better to #ifdef out the relevant code rather than strip it entirely—presumably it is useful to someone, and they can enable it at their own risk if it's that much of a source of problems.

[ccawley2011]

For reference, I asked about this in the Open Watcom V2 Discord, and this is the response I got:

ccawley2011 — Today at 16:09 Does OpenWatcom have an equivalent to __attribute__((no_reorder)) in GCC?

Jiri Malak — Today at 16:17 I am not aware about such possibility, but it depends for what you are using it. (edited)

ccawley2011 — Today at 16:27 Looking at the manual suggests that the standard method for creating interrupt handlers involve adding a dummy function after the interrupt handler to measure the size, however there seem to be some concerns about the possibility of the compiler reordering functions in a way that would cause this to not work. [16:28] There's some existing discussion here: https://github.com/libxmp/xmp-cli/pull/49

Jiri Malak — Today at 16:32 Oh yes, it is code generator optimization in such cases you can create 3 ordered segments, first one with begining label and last one with ending label, linker guarantie order of these segments and order of over all code. Next possibility could be to not use pipeline optimization, but I am not sure if it solve this problem.

Jiri Malak — Today at 16:41 I will try to look on the code and issue in details and we will see if some sophisticated solution is possible, anyway order in code can be achieved only by different segments grouped together (it is portable solution). (edited)

Jiri Malak — Today at 20:14 I have a bad news, OW code generator optimize code by sharing same code and moving labels to this common code, that not preserve labels order. It cannot be switch off only by disabling any optimization by compiler -od option. It is problem with all __irqN_end functions that they point to same location. Luckily it has dirty solution by using #pragma aux code which break such optimization then such code can not be common. below is code fragment for auxiliary #pragma aux nop function

#if defined(__WATCOMC__)
static void nop( void );
#pragma aux nop = "nop"
#endif
and code for __irqN_end
static void __irq##irqno##_end(void)  \
{                                     \
nop();                            \
}

It ensures order of both functions with small overhead of single byte nop instruction and don't need to change any optimization. It should work this way for any existing OW version. Other small optimization can be done by including folowing line into source code (dosutil.h).

#pragma aux disable = "cli" "mov eax,1"
#pragma aux enable = "sti" "mov eax,1"

and removing appropriate functions from dosutil.c (edited)

Any objections to me applying the suggested changes? (ccawley2011/xmp-cli#1)

[sezero]

For watcom side, why not, just apply. However, as far as I can see, the only issue that it will fix will be preventing combining of the xxx_end label procedures, can you not do it like this instead, i.e. by defining only one _end procedure?

static struct {
    irq_handler handler;
    void (*end)();
} __irq_handlers[16] = {
    { __irq0_handler, __irq1_handler },
    { __irq1_handler, __irq2_handler },
    { __irq2_handler, __irq3_handler },
    { __irq3_handler, __irq4_handler },
    { __irq4_handler, __irq5_handler },
    { __irq5_handler, __irq6_handler },
    { __irq6_handler, __irq7_handler },
    { __irq7_handler, __irq8_handler },
    { __irq8_handler, __irq9_handler },
    { __irq9_handler, __irq10_handler },
    { __irq10_handler, __irq11_handler },
    { __irq11_handler, __irq12_handler },
    { __irq12_handler, __irq13_handler },
    { __irq13_handler, __irq14_handler },
    { __irq14_handler, __irq15_handler },
    { __irq15_handler, __irq_end }
};

GCC side still remains: The no_reorder attribute seems to be only available for __GNUC__ >= 5 ...

[sezero]

OK, is the following patch good? patch3.txt

[sezero]

OK, is the following patch good? patch3.txt

@ccawley2011: PING?

[sezero]

OK, is the following patch good? patch3.txt

@ccawley2011: PING?

Inlining the patch below for easier review:

diff --git a/src/dos/dosirq.c b/src/dos/dosirq.c
index 916def6..0a2a1dc 100644
--- a/src/dos/dosirq.c
+++ b/src/dos/dosirq.c
@@ -129,7 +129,7 @@ static void _free_iret_wrapper(_go32_dpmi_seginfo * info)
    free((void *)info->pm_offset);
 }

-struct irq_handle *irq_hook(int irqno, irq_handler handler, void (*end)())
+struct irq_handle *irq_hook(int irqno, irq_handler handler, irq_handler end)
 {
    int interrupt;
    struct irq_handle *irq;
@@ -215,7 +215,7 @@ void irq_unhook(struct irq_handle *irq)

 #elif defined(__WATCOMC__)

-struct irq_handle *irq_hook(int irqno, irq_handler handler, void (*end)())
+struct irq_handle *irq_hook(int irqno, irq_handler handler, irq_handler end)
 {
    unsigned long size = (char *)end - (char near *)handler;
    int intno = (irqno > 7) ? (irqno + 104) : (irqno + 8);
@@ -290,9 +290,6 @@ static void INTERRUPT_ATTRIBUTES NO_REORDER __irq##irqno##_handler ()                       \
     __irq_mask |= (1 << irqno);                                \
   }                                                            \
   irq_ack (__irqs [irqno]);                                    \
-}                                                          \
-static void NO_REORDER __irq##irqno##_end(void)                                \
-{                                                          \
 }

 /* *INDENT-OFF* */
@@ -312,28 +309,28 @@ DECLARE_IRQ_HANDLER(12)
 DECLARE_IRQ_HANDLER(13)
 DECLARE_IRQ_HANDLER(14)
 DECLARE_IRQ_HANDLER(15)
+static void INTERRUPT_ATTRIBUTES NO_REORDER __irq_end(void) { }
 /* *INDENT-ON* */

 static struct {
-   irq_handler handler;
-   void (*end)();
+   irq_handler handler, end;
 } __irq_handlers[16] = {
-   { __irq0_handler, __irq0_end },
-   { __irq1_handler, __irq1_end },
-   { __irq2_handler, __irq2_end },
-   { __irq3_handler, __irq3_end },
-   { __irq4_handler, __irq4_end },
-   { __irq5_handler, __irq5_end },
-   { __irq6_handler, __irq6_end },
-   { __irq7_handler, __irq7_end },
-   { __irq8_handler, __irq8_end },
-   { __irq9_handler, __irq9_end },
-   { __irq10_handler, __irq10_end },
-   { __irq11_handler, __irq11_end },
-   { __irq12_handler, __irq12_end },
-   { __irq13_handler, __irq13_end },
-   { __irq14_handler, __irq14_end },
-   { __irq15_handler, __irq15_end }
+   { __irq0_handler,  __irq1_handler  },
+   { __irq1_handler,  __irq2_handler  },
+   { __irq2_handler,  __irq3_handler  },
+   { __irq3_handler,  __irq4_handler  },
+   { __irq4_handler,  __irq5_handler  },
+   { __irq5_handler,  __irq6_handler  },
+   { __irq6_handler,  __irq7_handler  },
+   { __irq7_handler,  __irq8_handler  },
+   { __irq8_handler,  __irq9_handler  },
+   { __irq9_handler,  __irq10_handler },
+   { __irq10_handler, __irq11_handler },
+   { __irq11_handler, __irq12_handler },
+   { __irq12_handler, __irq13_handler },
+   { __irq13_handler, __irq14_handler },
+   { __irq14_handler, __irq15_handler },
+   { __irq15_handler, __irq_end }
 };

 void irq_detect_start(unsigned int irqs, int (*irq_confirm) (int irqno))
diff --git a/src/dos/dosirq.h b/src/dos/dosirq.h
index 66bd4c4..69bf766 100644
--- a/src/dos/dosirq.h
+++ b/src/dos/dosirq.h
@@ -17,8 +17,8 @@
 #define PIC1_BASE  0x20        /* PIC1 base */
 #define PIC2_BASE  0xA0        /* PIC2 base */

-#ifdef __GNUC__
-#define NO_REORDER __attribute__((no_reorder))
+#if defined(__GNUC__) && (__GNUC__ >= 5)
+#define NO_REORDER __attribute__((no_reorder,no_icf,noinline,noclone))
 #else
 #define NO_REORDER
 #endif
@@ -107,7 +107,7 @@ static inline int irq_check(struct irq_handle * irq)

 /* Hook a specific IRQ; NOTE: IRQ is disabled upon return, irq_enable() it */
 extern struct irq_handle *irq_hook(int irqno, irq_handler handler,
-                                   void (*end)());
+                                   irq_handler end);
 /* Unhook a previously hooked IRQ */
 extern void irq_unhook(struct irq_handle * irq);
 /* Start IRQ detection process (IRQ list is given with irq mask) */
diff --git a/src/dos/dossb.c b/src/dos/dossb.c
index 223b089..bd7c221 100644
--- a/src/dos/dossb.c
+++ b/src/dos/dossb.c
@@ -42,6 +42,13 @@ _func_noclone
    inportb(SB_DSP_RESET);
 }

+#if defined(__WATCOMC__)
+static void nop (void);
+#pragma aux nop = "nop"
+#else
+#define nop()
+#endif
+
 static void INTERRUPT_ATTRIBUTES NO_REORDER sb_irq()
 {
    /* Make sure its not a spurious IRQ */
@@ -68,8 +75,9 @@ static void INTERRUPT_ATTRIBUTES NO_REORDER sb_irq()
        sb.timer_callback();
 }

-static void NO_REORDER sb_irq_end()
+static void NO_REORDER INTERRUPT_ATTRIBUTES sb_irq_end()
 {
+   nop();
 }

 static boolean __sb_reset()
@@ -127,8 +135,9 @@ static void INTERRUPT_ATTRIBUTES NO_REORDER __sb_irq_dmadetect()
    irq_ack(sb.irq_handle);
 }

-static void NO_REORDER __sb_irq_dmadetect_end()
+static void INTERRUPT_ATTRIBUTES NO_REORDER __sb_irq_dmadetect_end()
 {
+   nop();
 }

 static boolean __sb_detect()
diff --git a/src/dos/dosutil.c b/src/dos/dosutil.c
index 1d575f2..26c3c99 100644
--- a/src/dos/dosutil.c
+++ b/src/dos/dosutil.c
@@ -2,7 +2,7 @@

 #if defined(__DJGPP__)

-#include <go32.h> /* includes sys/version.h (djgpp >= 2.02) */
+#include <sys/version.h>
 #include <dpmi.h>
 #include <sys/nearptr.h>

@@ -10,7 +10,7 @@
  * src/libc/dpmi/api/d0102.s loads the selector and allocsize
  * arguments in the wrong order.  DJGPP >= 2.02 have it fixed. */
 #if (!defined(__DJGPP_MINOR__) || (__DJGPP_MINOR__+0) < 2)
-#warning __dpmi_resize_dos_memory() from DJGPP <= 2.01 is broken!
+#error __dpmi_resize_dos_memory() from DJGPP <= 2.01 is broken!
 #endif

 int dpmi_allocate_dos_memory(int paragraphs, int *ret_selector_or_max) {
diff --git a/src/dos/dosutil.h b/src/dos/dosutil.h
index 1d2eccf..3d22e34 100644
--- a/src/dos/dosutil.h
+++ b/src/dos/dosutil.h
@@ -13,11 +13,14 @@ extern int dpmi_unlock_linear_region_base(void *address, unsigned long size);
 #ifdef __WATCOMC__
 #include <conio.h>

+extern int disable();
+extern int enable();
+#pragma aux disable = "cli" "mov eax,1"
+#pragma aux enable = "sti" "mov eax,1"
+
 #define inportb(x)    inp(x)
 #define outportb(x,y) outp(x,y)

-extern int enable();
-extern int disable();
 #else
 #include <pc.h>
 #endif

[ccawley2011]

OK, is the following patch good? patch3.txt

This has been applied, with some minor changes to more closely match ccawley2011/xmp-cli#1.

[sezero]

If nothing else is left, I'm ready to merge this.

[sezero]

This is in now.

[sezero]

@ccawley2011: Unless you object, I plan to integrate your watcom changes back to libmikmod which is under LGPL. OK?

[ccawley2011]

@ccawley2011: Unless you object, I plan to integrate your watcom changes back to libmikmod which is under LGPL. OK?

Yes, that's fine.

[sezero]

@ccawley2011: Unless you object, I plan to integrate your watcom changes back to libmikmod which is under LGPL. OK?

Yes, that's fine.

Thanks.

[sezero]

Doing fine so far adapting watcom changes back to libmikmod.

From wss code, I get these warnings without the following:

../drivers/dos/doswss.c(298): Warning! W111: Meaningless use of an expression
../drivers/dos/doswss.c(308): Warning! W111: Meaningless use of an expression

(Remember that we define _farnspeekl for watcom as #define _farnspeekl(addr) (*((unsigned long *)(addr))) )

Is this correct?

diff --git a/libmikmod/drivers/dos/doswss.c b/libmikmod/drivers/dos/doswss.c
index 41cdf38..1708e62 100644
--- a/libmikmod/drivers/dos/doswss.c
+++ b/libmikmod/drivers/dos/doswss.c
@@ -282,7 +295,9 @@ static boolean __wss_detect()
        /* Prepare timeout counter */
        _farsetsel(_dos_ds);
        timer = _farnspeekl(0x46c);
-       while (timer == _farnspeekl(0x46c));
+       while (timer == _farnspeekl(0x46c)) {
+           nop();
+       }
        timer = _farnspeekl(0x46c);

        /* Reset all IRQ counters */
@@ -292,7 +307,9 @@ static boolean __wss_detect()
        __wss_regbit_set(WSSR_IFACE_CTRL, WSSM_PLAYBACK_ENABLE);

        /* Now wait 1/18 seconds */
-       while (timer == _farnspeekl(0x46c));
+       while (timer == _farnspeekl(0x46c)) {
+           nop();
+       }
        __wss_regbit_reset(WSSR_IFACE_CTRL, WSSM_PLAYBACK_ENABLE);
        dma_disable(wss.dma);

[sezero]

Another curios thing is __irq_stack_count (number of nested interrupts that can be handled): Specifically for gus, djgpp code sets it to 4 before calling irq_hook and then resets it to 1. Should we handle this in watcom code?

[sezero]

OK, here are patches to libmikmod (and mikmod) for dos / watcom: @ccawley2011, please review?

mikmod-patches.tar.gz

• I left one build failure in there on purpose for now: __irq_stack_count will be a missing symbol unless you move it out of #ifdef __DJGPP__: it is referenced by dosgus.c: I don't have a gus setup, so I can't test moving that out of ifdef or any other solution; but what does your eye- balling say about it?

• One of the libmikmod example programs, e.g. examples/simpleplay/splay.c builds and runs fine for me using sb driver (tested under dosbox, have yet to test on real hardware.)

• Mikmod player doesn't build yet: dosvideo.inc contents must be ported from djgpp conio/screen functions to, possibly, watcom's graph.h stuff. Don't know anything about that, so if you can do it then be my guest.

• Some of the stuff I noted above maybe (or not) still needs investigating.

[sezero]

Cameron: Have you had any chance to look at those libmikmod stuff I noted above?