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

Added LSPCON driver support and the fix for infinite loop on establishing HDMI connections on IGPU #24

Merged
merged 26 commits into from
Jun 25, 2019

Conversation

0xFireWolf
Copy link
Contributor

This pull request contains two fixes.

  • Add the driver support for onboard LSPCON chips to enable the DisplayPort to HDMI 2.0 output on some platforms.
    Brief Description:
    Recent laptops are equipped with a HDMI 2.0 port now. HDMI 2.0 ports on some laptops are routed to IGPU, but Intel IGPU does not provide native HDMI 2.0 output, so OEMs add a chip named LSPCON on the motherboard to convert DP signals to HDMI 2.0 signals.
    LSPCON chip has two running modes, and this driver automatically configures the adapter to work in the mode that is capable of converting DP to HDMI 2.0 signals, because some adapters are configured in the firmware to work in the HDMI 1.4 mode by default.
    Detailed description could be found at here in kern_igfx.cpp
  • Fix the infinite loop when the graphics driver tries to establish a HDMI connection with a higher pixel clock rate, e.g. 533.25 MHz for 4K @ 60Hz.
    Brief Description:
    This fix is intended to fix the HDMI 2.0 output issue on my Dell XPS 15 laptop and is now succeeded by the LSPCON driver. However, this fix might still be useful for those who want to have limited 2K/4K experience (i.e. 2K@59Hz or 4K@30Hz) with their HDMI 1.4 port.
    Detailed description could be found at here in kern_igfx.cpp

Relevant discussion thread: https://www.tonymacx86.com/threads/fix-coffee-lake-intel-uhd-graphics-630-on-macos-mojave-hdmi-output-issue-public-testing-stage.275126/

Articles related to this research is still working in progress.

Thanks,
FireWolf

Conflicts:
	WhateverGreen/kern_igfx.cpp
	WhateverGreen/kern_igfx.hpp
@vit9696
Copy link
Collaborator

vit9696 commented Jun 22, 2019

Hey,

Thanks for this pull-request. Looks quite impressive! It will take me some time to review the code, but all in all it looks reasonably healthy. For the time being

  • I noticed that there is some stuff like OSDynamicCast(OSNumber, framebuffer->getProperty("IOFBDependentIndex"))->unsigned32BitValue();, which is somewhat dangerous to even though in most cases it will likely work just fine.
  • -igfxhdmidivs is not backed with an IGPU property, perhaps it is worth doing that for parity?

@0xFireWolf
Copy link
Contributor Author

Thanks for your reply. Take your time. :)

I noticed that there is some stuff like OSDynamicCast(OSNumber, framebuffer->getProperty("IOFBDependentIndex"))->unsigned32BitValue();, which is somewhat dangerous to even though in most cases it will likely work just fine.

Yes, in theory it is dangerous, but the property value of IOFBDependentIndex always exists and therefore is non-null.

-igfxhdmidivs is not backed with an IGPU property, perhaps it is worth doing that for parity?

Honestly, I was thinking about adding a property for it before.
Since this fix could be replaced by the LSPCON driver in most cases, I think we don't need to add a property for it. Most people would prefer 4K@60Hz with HDMI 2.0 to 4K@24Hz with HDMI 1.4, so if they really need to apply this fix, they could just add an extra boot-arg.
For those who use 1080p HDMI display, they don't even need this. That's why Apple's original implementation works with common pixel clock rates under HDMI 1.4 standard.

However, considering the consistency, we could add a property for it. It doesn't hurt. What's your opinion?

WhateverGreen/kern_igfx.cpp Outdated Show resolved Hide resolved
WhateverGreen/kern_igfx.cpp Outdated Show resolved Hide resolved
WhateverGreen/kern_igfx.cpp Outdated Show resolved Hide resolved
WhateverGreen/kern_igfx.cpp Outdated Show resolved Hide resolved
WhateverGreen/kern_igfx.cpp Outdated Show resolved Hide resolved
WhateverGreen/kern_igfx.hpp Outdated Show resolved Hide resolved
WhateverGreen/kern_igfx.hpp Outdated Show resolved Hide resolved
WhateverGreen/kern_igfx.hpp Outdated Show resolved Hide resolved
WhateverGreen/kern_igfx.hpp Outdated Show resolved Hide resolved
WhateverGreen/kern_igfx.cpp Outdated Show resolved Hide resolved
@vit9696
Copy link
Collaborator

vit9696 commented Jun 24, 2019

Sorry for the delay, I believe I am done now. I marked the necessary changes inline and mentioned below.

  1. For some reason select files got their permissions changed (e.g. 100755 → 100644 for WhateverGreen/kern_igfx.cpp). Was this intentional and what is the right one you think? The change disables writing for anything but the owner, so I get a feeling it may not be desired. If it is a concern, best to address it for all files in a separate patch.

  2. I also discovered that Apple-doc comments seem to have tabs after *, i.e.

    /*
     * <tab here> Text
     */
    

    The rest of the code uses spaces, so it is probable worth changing it for consistency.

  3. -igfxhdmidivs I think it probably still makes sense to make it property-controlled. Not much work but better for consistency. Not so long ago I made the NVIDIA property & arg based exactly for that reason.

@0xFireWolf
Copy link
Contributor Author

Thanks for reviewing my code. I have pushed a new commit.

1. For some reason select files got their permissions changed (e.g. 100755 → 100644 for WhateverGreen/kern_igfx.cpp). Was this intentional and what is the right one you think? The change disables writing for anything but the owner, so I get a feeling it may not be desired. If it is a concern, best to address it for all files in a separate patch.

No, this wasn't intentional. All my private files have permissions 644. I have reverted back to 755 for files in WEG.

2. I also discovered that Apple-doc comments seem to have tabs after `*`, i.e.
   ```
   /*
    * <tab here> Text
    */
   ```
   
   
   The rest of the code uses spaces, so it is probable worth changing it for consistency.

Done.

3. `-igfxhdmidivs` I think it probably still makes sense to make it property-controlled. Not much work but better for consistency. Not so long ago I made the NVIDIA property & arg based exactly for that reason.

Done.

@0xFireWolf
Copy link
Contributor Author

By the way, I am considering implementing a helper to retrieve the framebuffer index in a convenient way.
Something like the following.

struct AppleIntelFramebufferExplorer {
    static bool getIndex(IORegistryEntry *framebuffer, uint32_t &index) {
         auto idxnum = OSDynamicCast(OSNumber, framebuffer->getProperty("IOFBDependentIndex"));
         if (idxnum != nullptr) {
             index = idxnum->unsigned32bitValue();
             return true;
         } else {
             return false;
         }
    }
}

What's your suggestion?

@vit9696
Copy link
Collaborator

vit9696 commented Jun 24, 2019

By the way, I am considering implementing a helper to retrieve the framebuffer index in a convenient way.

Something like that could do, as you see fit I guess.

WhateverGreen/kern_igfx.cpp Outdated Show resolved Hide resolved
WhateverGreen/kern_igfx.cpp Outdated Show resolved Hide resolved
WhateverGreen/kern_igfx.cpp Outdated Show resolved Hide resolved
WhateverGreen/kern_igfx.hpp Outdated Show resolved Hide resolved
@vit9696
Copy link
Collaborator

vit9696 commented Jun 24, 2019

All in all this looks quite good to me aside the small nuances I added. Please fix them up and I will merge as soon as I can. Thank you so much for this awesome stuff!

@0xFireWolf
Copy link
Contributor Author

All in all this looks quite good to me aside the small nuances I added. Please fix them up and I will merge as soon as I can. Thank you so much for this awesome stuff!

I have pushed new commits to the repo. Thanks again for reviewing my code.

Copy link
Collaborator

@vit9696 vit9696 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, found a small issue I did not spot initially. The rest looks good.

WhateverGreen/kern_igfx.cpp Outdated Show resolved Hide resolved
@vit9696 vit9696 merged commit 8689a5c into acidanthera:master Jun 25, 2019
@vit9696
Copy link
Collaborator

vit9696 commented Jun 25, 2019

Thank you! Merged!

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