LiveView hacks (write speed improvement)

Started by theBilalFakhouri, April 07, 2022, 06:20:22 AM

Previous topic - Next topic

0 Members and 1 Guest are viewing this topic.

names_are_hard

I suppose it depends what you mean by "doing stubs wrong" :)  In the module code, where it's just an address, I haven't checked how it's used.  In a stub context, it will be treated as a function pointer according to the definition of NSTUB (or ARM32_FN / THUMB_FN if you use those).  Is it appropriate to treat these addresses in that way?  That depends how the module uses them.  In the exact text you gave, you put them in as comments, which will do nothing (and the symbol won't get exported, so the modules won't find it).

Quote
more_hacks_are_supported is basically just to hide the option from the menu in cams that don't support.  Is there a more elegant way to accomplish this?

Probably, but that's a bigger question than you might realise.  I think the intention when ML added lots of supported models was that they'd all get all the features eventually.  Later on (I guess) as you've found, FEATURE and CONFIG limit or specify features, but that doesn't work well with modules.  Modules are *supposed* to be cam independent, but actually this was never true, there were some edge cases that became apparent when we worked more on modern cams and found that some structs and other internals had changed enough that some assumptions around how modules worked failed.

So, what's an elegant way to let modules behave the right way on a range of cams?  My personal feeling is modules could, instead of doing this:


if (is_camera("5D3", "1.2.3"))
{
    screen_x = 0x0101; screen_y = 0x0202;
}
else if { // many other cameras omitted }


Do something like this:

    get_screen_x_y(screen) // screen is a struct with .x and .y members


That is: cleanly separate modules from ML code, so they can only work via exported functions.  Modules ask the cam what the right values are for that model.  No hard-coding values inside modules.

So in your specific example, create a stub for *all* cams maybe called is_more_hacks_supported() (this is a bad name, but you get the idea).  This would probably default to returning false, and be overridden by cams that did have support.  Then the module code is greatly simplified - no per cam checks, but something like this:


    more_hacks_supported = is_more_hacks_supported();


Is this the best way to go?  That's harder to say.  It would increase the size of the ML binary, while reducing the size of modules.  Size constraints are a real concern on some cams.

dpjpandone

Quote from: names_are_hard on January 31, 2023, 04:18:17 AM

Is this the best way to go?  That's harder to say.  It would increase the size of the ML binary, while reducing the size of modules.  Size constraints are a real concern on some cams.

I have created a new post in General Development in order to keep this thread on topic:

https://www.magiclantern.fm/forum/index.php?topic=26779.new#new

theBilalFakhouri

Quote from: names_are_hard on January 28, 2023, 03:17:21 PM
Are these addresses function pointers?  If so, you shouldn't really cast to void *.  This will hide the compiler warning without fixing the problem.

How many params do these functions take?  Do they return a value?  If they don't meet standard ARM calling conventions this could corrupt program state and maybe cause crashes.
...

I wasn't aware of these, thanks for mentioning them, will look into it later.
Yes, I think all of them return value, what should I do in this case?

Also it seems lvfaceEnd and aewbSuspend take values, but not quite sure, but might be decompilation error in Ghidra, how to make sure?
I used "(void *)" blindly, I saw a1ex used it in sd_uhs for SD_ReConfiguration function, I just reused it and didn't notice problems . .


theBilalFakhouri

Quote from: names_are_hard on January 28, 2023, 04:37:23 PM
Nobody should be using magiclantern_hg_02 and I don't know why Bilal is working from that repo :)  I made that one strictly as an archive, it keeps all the history of the hg repo.  I will never take changes back into https://github.com/reticulatedpines/magiclantern_hg_02

I used it because I wanted a github solution from official hg repo which include all history beside it's a repo that well tested (on DIGIC 5), I didn't want to work on "magiclantern_simplified" because it has a lot of changes, and it still not well tested on DIGIC 5, I just didn't want to have some random or hidden surprises and spend time on it . . this thing is temporarily, my final goal is to port everything back in one branch and repo like magiclantern_simplified of course.

Quote from: names_are_hard on January 28, 2023, 04:37:23 PM
So, I think somebody needs to fix the problems crop_rec branch has on digic 4 cams, but I don't have any of those cams to test on.

Working on DIGIC 4 and give them crop_rec support is one of my goals, but it's not the highest priority for me currently . .

names_are_hard

Quote from: theBilalFakhouri on January 31, 2023, 05:35:45 PM
Yes, I think all of them return value, what should I do in this case?

Also it seems lvfaceEnd and aewbSuspend take values, but not quite sure, but might be decompilation error in Ghidra, how to make sure?
I used "(void *)" blindly, I saw a1ex used it in sd_uhs for SD_ReConfiguration function, I just reused it and didn't notice problems . .

Here's the definition in that case:

static int (*SD_ReConfiguration)() = 0;


That is, SD_ReConfiguration is a function pointer to a function that takes no arguments (I think?  I didn't check what the implied arguments are if you leave that empty), and returns an int.  It is initialised to untyped 0, which is probably not ideal (does it give a compile error?).

Taking values is okay, the concern is potential conflict between the calling convention you declare (perhaps implicitly) in ML code, and the calling convention used by the external function.  ARM has a standard calling convention: https://stackoverflow.com/questions/261419/what-registers-to-save-in-the-arm-c-calling-convention.  Compilers will normally generate code according to those rules, but they don't have to, and it is possible to override the compiler with e.g. pragmas.  Certainly, some Canon code doesn't use this convention.

Let's imagine you have some generated ML code that wants to call a Canon function you've named "do_stuff()", and you've told ML uses standard ARM calling convention.  This means the compiler will generate assembly which assumes the following rules are true (and other rules, this is a specific example):
Quote
r4-r8 are callee-save registers
r9 might be a callee-save register or not (on some variants of AAPCS it is a special register)
r10-r11 are callee-save registers

"Callee-save" means the caller expects those registers will not be changed by the function call.  This why if you look at the disassembly in most cases you'll see them being saved at the start of a function and restored at the end.

Therefore, the compiler will assume that the values in some variables, etc, will never be changed before and after the call.  But if the Canon function is unusual, those can change.  If you don't tell ML this is one of those functions, this kind of thing can happen:


some_magiclantern_variable = 0;  // the compiler *might* choose to put this in r4
some_canon_function(); // in C, this function cannot change your variable...
if (some_magiclantern_variable != 0)
    // this should never happen, let's crash!
    assert(-1);  // but in asm, it certainly can, by changing the content of r4 and returning, and you assert here


In that code you'd expect the assert to never trigger.  But it can if conditions are right.  There are other edge cases.  Some functions don't return.  Some functions expect the return address is in register you're passing to it.  Some expect they can return to your saved return address.  This doesn't hurt you very often, but if it does it's really confusing.

So, you need to make sure that what you tell ML about Canon functions is correct.  Most Canon functions use standard ARM calling convention, which should work with the defaults.  Some do not and you should confirm this before using them.  You can then instruct the compiler to do the right thing, e.g. in dryos.h:

extern void __attribute__((noreturn)) cstart(void);

names_are_hard

Quote from: theBilalFakhouri on January 31, 2023, 05:45:53 PM
I used it because I wanted a github solution from official hg repo which include all history beside it's a repo that well tested (on DIGIC 5), I didn't want to work on "magiclantern_simplified" because it has a lot of changes, and it still not well tested on DIGIC 5, I just didn't want to have some random or hidden surprises and spend time on it . . this thing is temporarily, my final goal is to port everything back in one branch and repo like magiclantern_simplified of course.

Working on DIGIC 4 and give them crop_rec support is one of my goals, but it's not the highest priority for me currently . .

Makes sense.  I wasn't saying it was wrong of you to use it, just that I didn't know your reasons :)  It will make your future pain around porting higher, of course.

I don't require crop_rec to give Digic 4 cams crop_rec ability in order to take that code.  I require those cams to be no worse than before.  If they simply do nothing with crop_rec stuff that's fine by me.  If we can restrict them with a feature flag or similar, that's acceptable.  Is it possible to define what work needs to be done to make this branch possible to merge?  Defined so that it is possible to know how to test if the conditions are met.  What currently happens on digic 4 and 5 cams?  What problems exist?

I'd really like to integrate the code that is getting used.  I have multiple reasons:
- it's cool code that enables cool features! 
- having one repo that everyone can use is better than 6 repos and everyone has to wonder which is the right one for their camera, etc
- if people are using old cams with my repo I have more confidence I haven't broken old cams while adding support for new ones

The last point has some caveats: if people are using my repo on old cams, I'll want some more tests first!  It's had some limited testing on old cams and no new problems or bugs reported so far.