Skip to content
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

add framebuffer-conX-XXXXXXXX for platform-id specific connector replacement #7

Merged

Conversation

RehabMan
Copy link
Contributor

A slight enhancement to framebuffer-conX-alldata, where you can use framebuffer-conX-XXXXXXXX-alldata. The -XXXXXXXX makes the patch specific to the active ig-platform-id. Essentially, conX-XXXXXXXX-alldata, if found matching the current platform-id, overrides conX-alldata.

This allows you to have one set of injections, but different based on ig-platform-id.

In my NUC repo, I needed this as the ig-platform-id can vary depending on device-id (ig-platform-id injected via conditional code in SSDT-IGPU.aml), and I didn't want to inject the patching properties from the AML (would rather keep patching in config.plist for now using Devices/Arbitrary).

A similar capability might make sense for the other patching properties, but I didn't implement it as I have no need so far.

@vit9696
Copy link
Collaborator

vit9696 commented Sep 11, 2018

Hi,

I am afraid these are hacks fixings hacks.

The first thing I disliked about your previous change is that it annihilates the purpose of not using binary patches in the first place but rather specifying them semantically. However, it can somewhat be lived with, if you think of it as a short form.

This thing honestly feels broken and not so well designed. As you see, there already exists framebuffer-framebufferid, which allows one to explicitly mark the patches as specific to a certain framebuffer id. The idea behind this option is to provide a way to boot into your system when the patches are wrong (by providing igfxframe=0xffffffff or some other fallback framebuffer).

If you think of it, you could see that the whole approach is not designed to support multiple hardware configurations or BSPs if you like. You configure once for your own hardware, and use it freely next. Injecting numerous other properties not only pollutes the configuration and code, but makes it less obvious and dangerous. To add more to it, it basically breaks the model, as no other keys support framebuffer id customisation.

All in all, I am really against this change, and suggest you to abandon the idea, because it will unavoidably lead to confusion rather than the benefit. Configuration is not meant to be unified for different BSPs. If you absolutely need it, sure, it can be done via SSDT, but I would suggest a better option.

In my opinion, the right way to support similar but different BSPs is to define some common config.plist template and have different properties elsewhere. You could use some tool (e.g. jinja2) to generate its variations for each supported platform. This way you can avoid code duplication and maintenance difficulty as well as keep the kernel side clean. I suppose it will greatly expand the overall featureset, because it will allow having not only NUC platforms configured, but other devices (by perhaps other people) as well.

Vit

@RehabMan
Copy link
Contributor Author

re: short form vs. semantic patching...
Yes, the conX-alldata is a direct response to the unwieldy result of trying to do bulk patching the semantic way (in my case, 27 injects to 5).

re: framebuffer-fraembufferid...
framebuffer-framebufferid is not useful for this case... It allows no mechanism to have different conX patches for different ig-platform-id values within the same set of injections (the problem further stems from the design here in the first place [using a dictionary instead of array for the patch collection]).

re: not being able to boot due to wrong patches
Funny you should mention that, as I did have that situation recently and had to get out a Clover USB to be able to boot my system. I still haven't investigated why I was not able to boot just by tweaking options in Clover (invalid FakeID), ... I suspect WhateverGreen.kext was doing some things (maybe overriding Clover fake device-id inject) that it shouldn't be doing. I should have tried -liluoff, but forgot about it.

re: your comments on my NUC repo
I have used semantic patching where appropriate (you can look at my changes in the NUC repo, 'beta' branch, but for mass reorg of the framebuffer connectors it is simply not practical.
Not sure I understand your comments regarding "having not only NUC platforms configured..." My NUC repo is for NUC only, no plans to support any other similar platforms. That is up to those that have such hardware, not me.
Not sure what you mean by "...use it freely next" either??

Your suggestion of generating different config.plist files for different NUC models, unfortunately would require a more knowledgeable end-user and more work for me (scripts to create the different plists), which I'm not really willing at this point. I took a similar tack in the ProBook repo, and to be honest, I wish I never had.

My goals in transitioning to WhateverGreen (besides enabling patching on Mojave) is to simplify my set of patches, and avoid version dependencies (which is why I didn't want to use framebuffer-patchX properties). With conX-alldata and conX-XXXXXXXX-alldata, it allows a relatively strait-forward transition from KextsToPatch used previously.

I see these two alldata options as options for experts. Used carefully, and in the right scenario, they are the best tools for the job.

Hopefully, you can be open to different ways of working.

@vit9696 vit9696 merged commit 07b95da into acidanthera:master Sep 11, 2018
@vit9696
Copy link
Collaborator

vit9696 commented Sep 11, 2018

Well… while I strictly disagree with the idea behind this option and I would discourage any use of it, I am not completely opposed to it as the change is small and it strongly makes your life easier. Let's have this merged.

@RehabMan
Copy link
Contributor Author

FYI: Able to use semantic patching for the u430 config.plist without it being too unwieldy.

8 property injects replace 2 patches:
screen shot 2018-09-11 at 11 04 31 am
screen shot 2018-09-11 at 11 04 42 am

That is assuming I don't need more work to further nullify the 0204 connector. I'm hoping that just changing the index is good enough. It will leave the connector as ff040900 00040000 87000000. We'll see what happens when I test it.

Note: With Haswell, it is not really necessary to convert to WhateverGreen style patching as KextsToPatch still works, but testing nonetheless.

@RehabMan
Copy link
Contributor Author

Thanks for merging!

@vit9696
Copy link
Collaborator

vit9696 commented Sep 11, 2018

You are welcome. Also, FYI, Arbitrary is deprecated to Properties, which are stable now.

@RehabMan
Copy link
Contributor Author

RehabMan commented Sep 11, 2018

I like Arbitrary better than Properties (gfxutil avoidance, corresponds nicely to GetDevices log in misc/preboot.log) and it is what that repo uses (last I checked, you could not mix Properties and Arbitrary).

But yeah, one of the things on my list is to merge Clover changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
2 participants