search

litex-hub / litevideo

Small footprint and configurable video cores (Deprecated)
Other
70 stars 25 forks source link

Review: Regression Fix for 7-series HDMI Input and Output #22

Closed rohitk-singh closed 5 years ago

rohitk-singh commented 5 years ago

Hi @enjoy-digital,

I'm not expecting the full PR to get merged since it might break any design using "pix_o" from HDMI input directly to drive the output HDMI (even if I think it is not a good idea).

But at least, the commit 1e51823 can be merged without any major issues. It is a critical commit, without which the HDMI output will not work for 7-series devices.

The commit 411669b "re-reverts" the accidental revert which happened in commit 784cc8c. I've mentioned in commit message also.

The commit 17ebc03 requires some thorough review, and can possibly break some designs which directly use the clock from HDMI input to drive HDMI output. I've tried to explain the situation in the commit message.

I've tested this PR with Mimas A7's video target in @timvideos/HDMI2USB-litex-firmware repo, and both HDMI output as well as HDMI input are now working with these changes. Issue #18 should also get resolved with this PR. Your review is high appreciated!

rohitk-singh commented 5 years ago

@enjoy-digital I don't have access to Nexys Video or NeTV2 or any other 7-series board with HDMI. It would be great if you could check this PR on any/both of them also.

enjoy-digital commented 5 years ago

Hi @rohitk-singh,

thanks a lot for investigating and finding the root cause of this! As you mention, there is a regression, so i'm going to merge your changes now. It will not break the usual designs we are using. I still have some work to to integrate some changes found by @bunnie while working on the NeTV2, so i'll figure while doing that how to handle the points you are mentioning.

rohitk-singh commented 5 years ago

@enjoy-digital Thanks for merging the PR! :)

mithro commented 5 years ago

@rohitk-singh Do we need to pull this into HDMI2USB-litex-firmware / litex-buildenv?

rohitk-singh commented 5 years ago

@mithro Yes, we do. Updating the submodules should be sufficient.

mithro commented 5 years ago

Doing so at https://github.com/timvideos/HDMI2USB-litex-firmware/pull/467