-
Notifications
You must be signed in to change notification settings - Fork 34
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Yamaha YMF724/YMF744/YMF754 (DS-XG) driver with hardware FM/MPU-401 port remapping #81
Conversation
It's really great! please wait while I have a simple review. |
Thanks for your great work! |
Please let me know your suggestions. |
I added some review comments on your change, can you check it? EDIT: it's not related to linux drivers but to |
I looked all over but I can't find your comments. |
static uint32_t MAIN_OPL3_388(uint32_t port, uint32_t val, uint32_t out) | ||
{ | ||
return out ? OPL3EMU_PrimaryWriteIndex(val) : OPL3EMU_PrimaryRead(val); | ||
if (main_hw_fmport) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can determine main_hw_fmport
before initialization and install a totally different handler (i.e. MAIN_OPL3_388_HWREMAP() ) to avoid check main_hw_fmport
in runtime every time.
Also it will be better if main_hw_fmport
is put in mpxplay_audioout_info_s
struct, and the init routine will check it and use another trap handler (aka MAIN_OPL3_388_HWREMAP).
In such a way we decouple main from YMF driver, YMF driver only depend on mpxplay/au_cards module, not an extern defined in main.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it a problem for some slow machines if you check main_hw_fmport
every time?
I think it is OK to commit it as is now, and improve it later.
There needs to be a way to have two or more cards selected. For example YMF for FM/MIDI and NULL or HDA for sound effects.
You could have options like /SC for sound effects, /FC for FM, /MC for MIDI.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it a problem for some slow machines if you check
main_hw_fmport
every time?
No, it is not.
As previously mentioned, That is A MONIOR SUGGESTION, and is not required do modify it right now.
But I'll be much better if dependency of main_hw_fmport
is removed from main.c. The idea is to treat mpxplay/au_cards as a standalone library, and it should be SELF CONTAINED, WITHOUT ANY INCLUDE/LINKAGE dependencies.
Think about this: if anyone, by any chance, want to use mpxplay/au_cards, he need to define a variable name main_hw_fmport
to make the linker pass, which he might no even care; Or he need to modify the source code to remove anything involving main_hw_fmport
. Either way it is not a good portable code.
This is already encountered when porting mpxplay into SBEMU. you can find dummy variables in au_base.c
line 30:
//dummy symbol to keep original code unmodified
#ifdef __DOS__
#include <dos.h>
void (__far *oldint08_handler)();
volatile unsigned long int08counter;
unsigned long mpxplay_signal_events;
#endif
and au_cards.c, line 146
#ifdef SBEMU
struct mpxplay_audioout_info_s au_infos;
unsigned int playcontrol,outmode = OUTMODE_TYPE_AUDIO;
unsigned int intsoundconfig=INTSOUND_NOINT08|INTSOUND_NOBUSYWAIT,intsoundcontrol;
unsigned long allcpuusage,allcputime;
unsigned int is_lfn_support,uselfn,iswin9x;
#endif
It's understandable that the author of mpxplay didn't predict that his code will be used others, but we can avoid the same problem for SBEMU.
Also please be noted that still it is a minor suggestion and you don't have to change it now, or for yourself, but please confirm if you want change it now.
There needs to be a way to have two or more cards selected. For example YMF for FM/MIDI and NULL or HDA for sound effects.
Yes for now it is. I will be glad if you wanna add this, or if not, I can do that later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. Naturally I think you would add some routines card_fm_read, card_fm_write etc. to struct one_sndcard_info.
I can do this later, but I want to clean up the Linux drivers first.
It would be easier for me to merge to/from my fork if this YMF patch gets merged first.
Before this I didn't have any experience with DOS.
I thought since this is a TSR it needs to be fast/simple, and, looking at main.c, thought that a global variable (main_hw_fmport
) was OK.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, we can leave it for later. I'm really sorry for keep you waiting so long, I was digging into the doom crash problem and now still be. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is the problem with doom? With which card?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is the problem with doom? With which card?
The onboard HDA that @hjnijlunsing has, it's a compatibility problem not quite related to the soundcard driver as opl3 works.
Discussed here #13 (comment)
@@ -144,43 +184,57 @@ static uint32_t MAIN_MPU_330(uint32_t port, uint32_t val, uint32_t out) | |||
c = '\n'; | |||
mpu_dbg_ctr = 0; | |||
} | |||
DBG_Log("%02x%c", val, c); | |||
DBG_Logi("%02x%c", val, c); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DBG_Log/DBG_Logi will output log for release builds. do your need a debug log but without the source file/line information?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, a hex dump of the raw MIDI data. I think it is a nice feature.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I understand. So it is NOT needed for release builds?
You can add one more specific macro like _LOG() but without file/lines, which will be disabled in release build. Or I can add that and you may use it, we can do that later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It IS intended for release builds.
if you use the option /MDBG2 you can output the MIDI data over the debug serial port at 115200 baud, 3 times the MIDI serial rate of 38400 baud (different from normal MIDI which is 31500 baud.)
You can dump the actual MIDI data that games are using while the music actually being played, without having to intercept it physically.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, cool :)
EDIT: but be ware that when /MDB or /DBG is not specified, it will probably print on the screen, please confirm whether it is expected.
EDIT2: I overlooked the context, you've guarded it with defines, and has variables switches too, it is OK, sorry for that :).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it is OK to print to the screen if /MDBG is specified but /DBG is not.
BTW, the hexdump uses 3 characters for each MIDI data byte, two for the MIDI data byte, and one more for the space or newline. Ignoring the LF to CRLF conversion in serial.c.
EDIT: That is why you need 3 times the MIDI baud rate.
BTW, is there a good reason to have the LF to CRLF conversion and the baud rate set to 9600?
In my builds I remove the LF to CRLF conversion, since in my Linux terminal using cat or minicom I get extra blank lines with CRLF.
And 9600 is just too slow so I set it to 115200.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might depend on the terminal, for me it needs CRLF to properly turn to next line.
I don't know why the baud rate is set, you may ask @thp who should know about this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's probably fine to have 115200 and LF only. As @jiyunomegami mentioned, it's up to the terminal on the other side to be configured properly.
@@ -341,8 +350,17 @@ void AU_init(struct mpxplay_audioout_info_s *aui) | |||
goto err_out_auinit; | |||
} | |||
if(!aui->card_handler && !(aui->card_controlbits&AUINFOS_CARDCNTRLBIT_SILENT)){ | |||
pds_textdisplay_printf("No supported soundcard found!"); | |||
goto err_out_auinit; | |||
if (au_cards_fallback_to_null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's good to use SBEMU on au_cards_fallback_to_null
in au_cards.h
in my perspective, if it is guarded in the header, it probably need a SBEMU switch in au_cards.c
too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added this commit:
commit 268bc96
Place au_cards_fallback_to_null within #ifdef SBEMU blocks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool, thanks
Sorry :), I forget submitting the reviews, now you can see it. |
BTW are you located in Japan? I guessed by your id, if you are, I think we share a similar time zone which is convenient for both of us :). |
Yes, Japan. I had some free time to work on this over the new year holidays (around Jan. 1st here.) I have a question: Is there a way to get a lower address without limiting it with himemx options etc? |
I remember there's a himemex2 wich allocates memory from low address. It comes with himemex in the original release, can you check that? |
Thanks. I do get lower addresses with himemx2. On this machine it is possible to allocate multiple times until you get a good address. I don't know if the BIOS has anything to do with it. with over 4GB of RAM it goes something like this: |
For the retry logic, I had to add |
Have you freed the previously allocated memory before retry? If not, then it will finally get more lower, but will leak/waste memories. If yes, I don't know why it behaves that way, it's weird. you might read the readme of himemex to know more about its allocation strategy. The original routine assume that if one allocation failed, there's something wrong with memory allocation or not enough memory, so it simply exists. I think you can return NULL but for other drivers it sill need exist from outside because the program is unable to run. |
OK. I will add a new function. I think it is OK to simply add a function that returns NULL on failure, since that is what the Linux drivers expect. |
Agreed. By the way of the retry, the memory cannot be freed before you get a valid address, but it will better to free them all after you get one. |
No description provided.