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

Potential violation of the HDMI 12 pixel minimum control period #43

Closed
krisdover opened this issue Aug 16, 2023 · 4 comments
Closed

Potential violation of the HDMI 12 pixel minimum control period #43

krisdover opened this issue Aug 16, 2023 · 4 comments
Labels
bug Something isn't working

Comments

@krisdover
Copy link

Hi Sameer, thanks so much for this amazing project. I've been porting bits of it into someone else's input lag tester project which I'm currently modifying to output proper HDMI (instead of DVI). I'm really just after the AVI InfoFrame functionality so I can force the HDMI sink I want to test (some FPV goggles) into RGB mode since it unfortunately defaults to expect the YCbCr video format. I've made good progress but I've noticed a discrepancy in the code which doesn't seem to match the HDMI Spec when it comes to the timing for the preamble:

Section 5.2.5.3 - Island Placement and Duration says:

All TMDS Control Periods shall be at least tS,min (12) characters (pixels) long.

Since the preamble is sent using a control period, it should always be preceded by at least a 4 pixel control period as show in the spec's timing diagram:

image

However the code seems to assume that data island preamble starts immediately after the video data period:

data_island_preamble <= num_packets_alongside > 0 && cx >= screen_width && cx < screen_width + 8;

Instead, souldn't this be:

data_island_preamble <= num_packets_alongside > 0 && cx >= screen_width + 4 && cx < screen_width + 12;

This of course would have flow effects for the calculations of guard period and data island period.

Anyhow, thanks again for all your hard work and apologies if I'm completely misinterpreting the spec. :)

@sameer
Copy link
Member

sameer commented Aug 17, 2023

Hi Kris,

Thanks for the detailed report, you're definitely correct. The data island preamble starts prematurely and runs for 8 pixels, when instead it should be delayed by >=4 to get the control period length of 12 pixels.

I'm surprised this has not come up before, maybe most sinks are a little forgiving on this.

There are a few things that need to change:

  • data_island_guard (8 to 12, 10 to 14)
  • data_island_preamble (0 to 4, 8 to 12)
  • data_island_period_instantaneous (10 to 14)
  • packet_enable (22 to 18)
  • max_num_packets_alongside -- I think this is slightly wrong, too. Need to remove v preamble / d preamble (-16), replace with additional control period (12). There's 4 extra pixels that are not considered

@sameer
Copy link
Member

sameer commented Aug 17, 2023

If you would be interested in creating a pull request for this, I would be happy to review it 🙂. Otherwise, I'll update the repo at some point.

Let me know how you'd like to proceed. Thanks again for finding this and reporting it

@sameer sameer added the bug Something isn't working label Aug 17, 2023
@sameer
Copy link
Member

sameer commented Aug 17, 2023

Looking through the issues, this could be the root cause of the issue @mopplayer was having in #35

krisdover pushed a commit to krisdover/hdmi that referenced this issue Aug 20, 2023
…deo and data-island preambles; fixed off-by-one errors on VG, VP, and VSync
@krisdover
Copy link
Author

Looking through the issues, this could be the root cause of the issue @mopplayer was having in #35

https://imgur.com/ArIkfPk

Yes, I was definitely getting the same 2-pixel white bar to the left of the display as reported in issue #35, that is until I fixed the minimum control period bug.

krisdover pushed a commit to krisdover/hdmi that referenced this issue Aug 20, 2023
…deo and data-island preambles; fixed off-by-one errors on VG, VP, and VSync; fixed test bench
@sameer sameer closed this as completed in dd3a1be Aug 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants