fkms/pi4 not responding to VC4_BROADCAST_RGB_LIMITED connector property

[alanbork]

fkms has a connector property for setting HDMI pixel encoding to RGB limited or RGB full:

https://github.com/raspberrypi/linux/blob/7fb9d006d3ff3baf2e205e0c85c4e4fd0a64fcd0/drivers/gpu/drm/vc4/vc4_firmware_kms.c#L1586

if you set that connector property using proptest

./proptest 89 connector 94 1 # full
./proptest 89 connector 94 2 # limited

it does nothing (contrast this with setting the margin, which takes effect immediately).

Based on this code:

https://github.com/raspberrypi/linux/blob/7fb9d006d3ff3baf2e205e0c85c4e4fd0a64fcd0/drivers/gpu/drm/vc4/vc4_firmware_kms.c#L951

it seems like the only time this property is referenced in the code is when a new mode is set - fine, it need not be immediate, even if that would be nice. BUT, even the next time a new video mode is initialized using kms/drm the limited/full property doesn't seem to have any effect on the pixel encoding actually sent over the HDMI cable.

if you go allllll the way back to a pi0 with the dispmanx firmawre and choose a dmt or cea mode using tvserice it works right there.

It's at least implied by #1579 (comment) that this should work for fkms (just not for regular kms), but it seems like it does not. Reading the code I don't see why it would fail, but maybe it's a subtle bug. Or maybe the bug is in the closed source firmware?

I realize fkms is on the back burner, but it's really unfortunate that there's no way at all to get the pi4 to output in broadcast (CEA) RGB rather than DMT with at least one of the kms or fkms driver sets.

[6by9]

There's a missing atomic_check function that sets the mode_changed flag in the drm_crtc_state should the broadcast_rgb property change. The equivalent of intel_digital_connector_atomic_check.
That'll then trigger a mode change whenever the broadcast_rgb property is changed.

This is why we really want this dealt with as a standard property rather than being hacked into every driver. It's far too easy to miss tiny parts of the detail.

Looking on an HDMI analyser, the AVI infoframe is changing to reflect the quantization range. It's decoded as "default - depends on video format" for limited mode, and as "full". That appears to be coming off Q1 and Q0 in byte PB3 which is set to 0x0 for limited range and 0x2 for full range. It currently never sets 0x1 for explicit limited range.
It is incorrect to explicitly signal limited mode when in a CEA mode (exception being mode 1 VGA which is defined as being full range).

The colour conversion matrices will have been updated to use limited mode appropriately.

There may be a further case that isn't considered. VIC=0 is effectively the flag for CEA vs DMT in the AVI infoframe. If VIC=0, then CEA rules go out the window, and it's reasonable to always set the range explicitly.

[alanbork]

Thanks for looking into this. Is there a usemode property to toggle the VIC? If I understand you correctly the flaw in the fkms code is that the limited range property change doesn't trigger a mode change immediately, but it does work otherwise? On my end it appears that I never get a limited range, but maybe what is really happening is the VIC is always set to indicate DMT. I can't measure raw HDMI pixel values directly, only the display's final conclusion about the signal, which is that it's a DMT encoded mode no mater what resolutions I choose or changes to VC4_BROADCAST_RGB_LIMITED. If I can set the VIC value either from the console or from usermode c, I'd like to try that.

…

On Wed, May 19, 2021, 2:45 AM 6by9 ***@***.***> wrote: There's a missing atomic_check function that sets the mode_changed flag in the drm_crtc_state should the broadcast_rgb property change. The equivalent of intel_digital_connector_atomic_check <https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/i915/display/intel_atomic.c#L147> . That'll then trigger a mode change whenever the broadcast_rgb property is changed. This is why we really want this dealt with as a standard property rather than being hacked into every driver. It's far too easy to miss tiny parts of the detail. Looking on an HDMI analyser, the AVI infoframe is changing to reflect the quantization range. It's decoded as "default - depends on video format" for limited mode, and as "full". That appears to be coming off Q1 and Q0 in byte PB3 which is set to 0x0 for limited range and 0x2 for full range. It currently never sets 0x1 for explicit limited range. It is incorrect to explicitly signal limited mode when in a CEA mode (exception being mode 1 VGA which is defined as being full range). The colour conversion matrices will have been updated to use limited mode appropriately. There may be a further case that isn't considered. VIC=0 is effectively the flag for CEA vs DMT in the AVI infoframe. If VIC=0, then CEA rules go out the window, and it's reasonable to always set the range explicitly. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#1580 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ANPGZ5LMSMOX6PNLN7FQO33TOOCDXANCNFSM445RQTZA> .

[alanbork]

All I can find about the VIC values is this rather old scanned document: https://archive.org/details/CEA-861-B/page/n71/mode/2up but that suggests it should be the entire CEA mode number. Is that what FKMS sends? I can imagine that the pi0 with dispmanx is sending the actual CEA code since that's how tvservice itself is initialized, but it seems like a significant bother to work backwards from the timing codes to the CEA mode number and perhaps fkms skips that? Or is that stored somewhere in the drm structure for the supported modes? This presentation on 4k ( https://events.static.linuxfound.org/sites/events/files/slides/elce2017.pdf) suggests that in practice the VIC values are somewhat haphazardly implemented... But perhaps samsung in particular has more stringent expectations for what is in that field, and that's why I'm getting strange behavior.

…

On Wed, May 19, 2021 at 7:28 AM AR ***@***.***> wrote: Thanks for looking into this. Is there a usemode property to toggle the VIC? If I understand you correctly the flaw in the fkms code is that the limited range property change doesn't trigger a mode change immediately, but it does work otherwise? On my end it appears that I never get a limited range, but maybe what is really happening is the VIC is always set to indicate DMT. I can't measure raw HDMI pixel values directly, only the display's final conclusion about the signal, which is that it's a DMT encoded mode no mater what resolutions I choose or changes to VC4_BROADCAST_RGB_LIMITED. If I can set the VIC value either from the console or from usermode c, I'd like to try that. On Wed, May 19, 2021, 2:45 AM 6by9 ***@***.***> wrote: > There's a missing atomic_check function that sets the mode_changed flag > in the drm_crtc_state should the broadcast_rgb property change. The > equivalent of intel_digital_connector_atomic_check > <https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/i915/display/intel_atomic.c#L147> > . > That'll then trigger a mode change whenever the broadcast_rgb property is > changed. > > This is why we really want this dealt with as a standard property rather > than being hacked into every driver. It's far too easy to miss tiny parts > of the detail. > > Looking on an HDMI analyser, the AVI infoframe is changing to reflect the > quantization range. It's decoded as "default - depends on video format" for > limited mode, and as "full". That appears to be coming off Q1 and Q0 in > byte PB3 which is set to 0x0 for limited range and 0x2 for full range. It > currently never sets 0x1 for explicit limited range. > It is incorrect to explicitly signal limited mode when in a CEA mode > (exception being mode 1 VGA which is defined as being full range). > > The colour conversion matrices will have been updated to use limited mode > appropriately. > > There may be a further case that isn't considered. VIC=0 is effectively > the flag for CEA vs DMT in the AVI infoframe. If VIC=0, then CEA rules go > out the window, and it's reasonable to always set the range explicitly. > > — > You are receiving this because you authored the thread. > Reply to this email directly, view it on GitHub > <#1580 (comment)>, > or unsubscribe > <https://github.com/notifications/unsubscribe-auth/ANPGZ5LMSMOX6PNLN7FQO33TOOCDXANCNFSM445RQTZA> > . >

[alanbork]

I can confirm that this is working properly. Sorry for the long delay, I didn't have access to hardware to test it properly again until now. Any chance of getting access to the VIC field so that it can also be set properly?

[alanbork]

Also, @mripard, what's the likelihood of this feature making it into the KMS driver anytime soon?

[6by9]

Any chance of getting access to the VIC field so that it can also be set properly?

The VIC is set by DRM should the mode chosen match one of the standard modes. That's done automatically by drm_hdmi_avi_infoframe_from_display_mode called from vc4_crtc_mode_set_nofb. That's then passed down to the firmware via mb.timings.video_id_code. Note that it also changes the behaviour of how the range is advertised in the AVI infoframe. My comments on the commit are

    FKMS will set the VIC for the mode if it is a CEA mode, so use that
    to determine whether to signal:
    - default/full (CEA modes except VGA (VIC>1))
    - limited/default (CEA VGA mode (VIC=1))
    - limited/full (DMT modes (VIC=0)) 

The VIC is never exposed to userspace as it makes no sense to do so - you could set a mismatch between the mode and the VIC which will potentially confuse displays.

Under what situation do you believe it would be helpful to set it explicitly?

what's the likelihood of this feature making it into the KMS driver anytime soon?

We had a discussion about adding "Broadcast RGB" to KMS last week.

Mainline folks are still having discussions about how it should be done as a generic control instead of an Intel i915 and GMA500 custom property. AMD had an alternative IIRC.

Patches so far have been to adopt the Intel "Broadcast RGB" property (although discussions about renaming it or not), so we are likely to add downstream patches implementing it before upstream come to their final decision, and we'll hope the two match.

[alanbork]

I think the AMD property is called output_csc, which is a pretty undescriptive name, I hope broadcast RGB wins over that!

the reason I want access to the VIC is purely for debugging - I've observed that several recent model Samsung TVs refuse to treat the pi4 as a regular video input, and (I think) instead treat it as a computer input. This results in the TVs allowing limited control over video settings (color correction, game mode lag reduction, etc, etc). Being able to observe the VIC value to determine if that's the issue would be a start, but likely having control over it would help even moreso. This is a pi4 specific issue as the pi0 with dispmanx has no problem with the samsung TVs and I'd like to figure out what the difference in the signal is - I thought it was the broadcast RGB range but that seems to not be it. I'm not sure this is a pi4 bug exactly, it may well be a dumb choice on samsung's part but without more information and control over the pi signals it's not possible to tell.

[6by9]

I seem to recall that the AMD output_csc property also allows selection of YUV/YCbCr formats over RGB, but that's really an independent control.

If you're debugging then I'd push you towards building the kernel with whatever additional logging you wish for. Adding the VIC as a property doesn't really help.

The one place that I think DRM may fall down is that it tries to match modes on flags as well as the timings. The detailed timings typically differ from the CEA modes in polarity of the sync pulses, eg for my Dell 4k monitor

  Detailed Timing Descriptors:
    DTD 1:  3840x2160   60.000 Hz  16:9   135.000 kHz 594.000 MHz (609 mm x 349 mm)
                 Hfront  176 Hsync  88 Hback 296 Hpol P
                 Vfront    8 Vsync  10 Vback  72 Vpol N
...
  Native detailed modes: 1
  Video Data Block:
    VIC  97:  3840x2160   60.000 Hz  16:9   135.000 kHz 594.000 MHz
    VIC  96:  3840x2160   50.000 Hz  16:9   112.500 kHz 594.000 MHz
    VIC  95:  3840x2160   30.000 Hz  16:9    67.500 kHz 297.000 MHz
    VIC  94:  3840x2160   25.000 Hz  16:9    56.250 kHz 297.000 MHz
    VIC  93:  3840x2160   24.000 Hz  16:9    54.000 kHz 297.000 MHz

VIC 97 is defined as

	{ DRM_MODE("3840x2160", DRM_MODE_TYPE_DRIVER, 594000, 3840, 4016,
		   4104, 4400, 0, 2160, 2168, 2178, 2250, 0,
		   DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC),

DTD says negative VSYNC. VIC says positive VSYNC. Different modes.
DRM will have taken both of those and added them to the list of modes. I get modetest reporting

  #0 3840x2160 60.00 3840 4016 4104 4400 2160 2168 2178 2250 594000 flags: phsync, nvsync; type: preferred, driver
  #1 3840x2160 60.00 3840 4016 4104 4400 2160 2168 2178 2250 594000 flags: phsync, pvsync; type: driver

So both modes exist, but the DTD version has been selected, and therefore the VIC field will be set to 0 as it is not the CEA mode.

Making DRM select the CEA mode in preference to the DTD is a different issue. X lists all the modes

HDMI-1 connected primary 3840x2160+0+0 (normal left inverted right x axis y axis) 609mm x 349mm
   3840x2160     60.00*+  60.00    50.00    59.94    30.00    25.00    24.00    29.97    23.98  

but I don't immediately know how you'd select that second 60.00Hz mode (the CEA one).

This is all code common to all Linux display drivers, not specific to the Pi.

The firmware has always preferred to use standard CEA or DMT modes, therefore it's been obvious what to set the VIC field to.

[6by9]

OK there is an easy way in X.

pi@raspberrypi:~ $ xrandr --verbose
Screen 0: minimum 320 x 200, current 3840 x 2160, maximum 7680 x 7680
HDMI-1 connected primary 3840x2160+0+0 (0x48) normal (normal left inverted right x axis y axis) 609mm x 349mm
...
  3840x2160 (0x48) 594.000MHz +HSync -VSync *current +preferred
        h: width  3840 start 4016 end 4104 total 4400 skew    0 clock 135.00KHz
        v: height 2160 start 2168 end 2178 total 2250           clock  60.00Hz
  3840x2160 (0x49) 594.000MHz +HSync +VSync
        h: width  3840 start 4016 end 4104 total 4400 skew    0 clock 135.00KHz
        v: height 2160 start 2168 end 2178 total 2250           clock  60.00Hz
  3840x2160 (0x4a) 594.000MHz +HSync +VSync
        h: width  3840 start 4896 end 4984 total 5280 skew    0 clock 112.50KHz
        v: height 2160 start 2168 end 2178 total 2250           clock  50.00Hz
  3840x2160 (0x4b) 593.407MHz +HSync +VSync

You can then use xrandr --output HDMI-1 --mode 0x49 to explicitly select the CEA mode, and having added logging into vc4_hdmi.c (I'm using vc4-kms-v3d) I see the VIC is being set to 97 with that.
Just select with xrandr --output HDMI-1 --mode 3840x2160 and I get mode 0x48 and a VIC of 0.

[alanbork]

but I don't immediately know how you'd select that second 60.00Hz mode (the CEA one).

if I'm following you properly, I could at least do it with modetest by selecting mode #1 using modetest (in your example, anyway), right? That would be enough to see if the VIC setting is the problem in my case, so long as I could actually determine if the mode I select changes the VIC being set. Which brings me to:

If you're debugging then I'd push you towards building the kernel with whatever additional logging you wish for. Adding the VIC as a property doesn't really help.

Well, it would help in that dumping properties is a single command line using proptest, whereas hacking the kernel requires a lot more than that; compiling your own kernel is at least well documented, though it is time consuming to setup. But knowing which source file and line is the right place to dump the VIC is definitely nontrivial. Lots of people have problems getting their display device to work with the pi (esp. when it comes to 4k) and not many of them are also kernel hackers. At the moment I don't even know if the issue is something on Samsung's side OR the pi's side OR the generic DRM code, although what you said about the DRM's VIC algorithms suggests that may well be the issue.

But in the end it sounds like this is a generic DRM issue not a pi issue, so I'm really reporting this to the wrong place it seems. Is there a github for DRM drivers? I've found the equivalent for modetest, but that doesn't seem to cover the driver level, and I don't think the main linux kernel mailing this wants to hear about my troubles with a single brand of TVs, even if its' one of the most popular ones.

Side note: many TVs (and other displays) are poorly behaved and idiosyncratic in my experience. I would argue that exposing the maximum number of knobs to twiddle is the best option for getting them all to play well with the pi, so long as those knobs are acknowledged as being the last resort.

[6by9]

if I'm following you properly, I could at least do it with modetest by selecting mode #1 using modetest (in your example, anyway), right? That would be enough to see if the VIC setting is the problem in my case, so long as I could actually determine if the mode I select changes the VIC being set. Which brings me to:

Yes modetest should do the same thing if you select by entry.

But knowing which source file and line is the right place to dump the VIC is definitely nontrivial.

drm_hdmi_avi_infoframe_from_display_mode should be used by any DRM driver (eg https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/vc4/vc4_hdmi.c#L390 and https://github.com/raspberrypi/linux/blob/rpi-5.10.y/drivers/gpu/drm/vc4/vc4_firmware_kms.c#L879)

But in the end it sounds like this is a generic DRM issue not a pi issue, so I'm really reporting this to the wrong place it seems. Is there a github for DRM drivers? I've found the equivalent for modetest, but that doesn't seem to cover the driver level, and I don't think the main linux kernel mailing this wants to hear about my troubles with a single brand of TVs, even if its' one of the most popular ones.

dri-devel mailing list - https://lists.freedesktop.org/mailman/listinfo/dri-devel
But I suspect that you won't get much traction there as it's really an issue of the client (ie X or fb emulation) needing to do something sensible. The kernel is already exposing all the relevant options.

Side note: many TVs (and other displays) are poorly behaved and idiosyncratic in my experience. I would argue that exposing the maximum number of knobs to twiddle is the best option for getting them all to play well with the pi, so long as those knobs are acknowledged as being the last resort.

But you have all the controls already. The only finessing would be altering how you select the mode. Exposing the VIC to userspace is the wrong answer.