xbmc
gbm: Set colourspace and bits per pixel
Description
Set colourspace and bits per pixel when playing high bit depth video
Motivation and context
Allows gbm platforms (like Pi 4) to drive 12-bit YCC422 over hdmi when playing
How has this been tested?
Tested on Pi 4 with 4K HDR display and with a HDMI analyser.
What is the effect on users?
Improved picture quality (less banding).
Screenshots (if appropriate):
Types of change
- [ ] Bug fix (non-breaking change which fixes an issue)
- [ ] Clean up (non-breaking change which removes non-working, unmaintained functionality)
- [X] Improvement (non-breaking change which improves existing functionality)
- [ ] New feature (non-breaking change which adds functionality)
- [ ] Breaking change (fix or feature that will cause existing functionality to change)
- [ ] Cosmetic change (non-breaking change that doesn't touch code)
- [ ] None of the above (please explain below)
Checklist:
- [X] My code follows the Code Guidelines of this project
- [ ] My change requires a change to the documentation, either Doxygen or wiki
- [ ] I have updated the documentation accordingly
- [ ] I have read the Contributing document
- [ ] I have added tests to cover my change
- [ ] All new and existing tests passed
For the PR to work you need https://github.com/raspberrypi/linux/pull/4201 (it should be a no-op without kernel support).
We are in a chicken and egg situation here - getting drm support for requesting the YCC422 into the upstream kernel is dependant on an open source piece of software agreeing to use it.
ping @lrusak
When playing 10-bit hevc:
$ kmsprint -p | egrep "COLOR|bpc|format "
output format (39) = 2 (YCrCb422) [RGB444=0|YCrCb444=1|YCrCb422=2|YCrCb420=3]
max bpc (40) = 12 [8 - 12]
COLOR_ENCODING (78) = 2 (ITU-R BT.2020 YCbCr) [ITU-R BT.601 YCbCr=0|ITU-R BT.709 YCbCr=1|ITU-R BT.2020 YCbCr=2]
COLOR_RANGE (79) = 0 (YCbCr limited range) [YCbCr limited range=0|YCbCr full range=1]
when video stops (or playing 8-bit)
$ kmsprint -p | egrep "COLOR|bpc|format "
output format (39) = 0 (RGB444) [RGB444=0|YCrCb444=1|YCrCb422=2|YCrCb420=3]
max bpc (40) = 8 [8 - 12]
COLOR_ENCODING (99) = 0 (ITU-R BT.601 YCbCr) [ITU-R BT.601 YCbCr=0|ITU-R BT.709 YCbCr=1|ITU-R BT.2020 YCbCr=2]
COLOR_RANGE (100) = 0 (YCbCr limited range) [YCbCr limited range=0|YCbCr full range=1]
I'm not really sure what the upstream consensus of this is but this seems overkill to have to set output format and max bpc from userspace. The intel driver seems to handle these things automatically by simply setting the plane format and the connector colorimetry.
Why does this driver need to do it from userspace?
https://github.com/raspberrypi/linux/pull/4280 was submitted upstream for feedback which I believe makes the selection automatically, but it sounds like userspace controlling output was preferred.
ping @mripard
I'll update this PR based on the newer kernel approach shortly. Basically the "max bpc" setting will remain (which is a standard drm property) but the "output format" (which would have been a new drm property) will be dropped (as that can be automatically decided by kernel driver).
I'd suggest requesting max_bpc=12, even if the video is only 10bit, as this will allow the video driver to use YCC422 - which is a 36-bit / 12 bit-per-component mode that doesn't fall into the deep-color (RGB444 / YCC444) category and is available on many (also older and non-deep-color capable) display devices.
eg RPi4 doesn't support RGB/YCC444 10 or 12bpc at 4kp60, but YCC422 is available.
Testing on LibreELEC/RPi4 looks fine here, also reports from our forum users testing builds with this PR is good
@lrusak sorry, this got forgotten about.
Updated this PR with more sanity checks that max bpc is available, supports the value we want and restores it back to original value.
I'm still not sure if those connector properties should be set from userspace at all - imho it should be set only kernel side - depending on the HW available. For Rockchip, for example, there is a SoC where the video decoder supports decoding 10 bit H.265 content but the display controller can output max 8 bit (via HDMI, which in theory could output 10 bit also, i.e >= HDMI 2.0) and has to downsample it before. As such deciding about max bpc connector property only based on video content is not sufficient - and seems to make no sense when thinking about drm architecture.
The kernel driver needs to know when it is beneficial to use a deep colour mode. There is no point using a deep colour mode when there is no content with more than 8 bits. And it should be avoided when not needed as it requires a higher clock rate, and so is more demanding on cable quality, and may require chroma downsampling.
And the "max bpc" connector property is already present and designed to be set from userspace. If a kernel driver doesn't want to expose a "max bpc" property or act on it, that is fine, and this PR won't cause any regression.
I'm fully aware, that the "max bpc" property is available already. It's just not enough to decide about that connector property only based on video content (which is a completly different component from architectural pov). You are completly free to loop over your active planes in your kernel driver and check format/colorspace and set the max bpc according to that.
It's tricky. We had several discussions about it before and the conclusion is userspace needs a way to hint the kernel which mode to choose as kernel doesn't know about the usecase and what the "best" mode for it would be.
Consider the following: you are running 4kp60 on an HDMI 2.0 monitor and have an 8bit RGB and a 10 (or 12) bit YCC plane.
Now what would be the best output mode? 8bit RGB/YCC 4:4:4 or 12bit YCC 4:2:2?
If you play a 10bit movie and have an 8bit plane with subtitles or OSD overlay then the answer would be 12bit YCC 4:2:2 to get best video reproduction quality (and it's OK if the 8bit subtitles get reduced chroma sampling).
But if you are working in a desktop environment, editing code etc and the 10bit plane is just some youtube video you are watching in a window or some video conference call the answer is 8bit RGB/YCC 4:4:4 as you wouldn't want the chroma subsampling to blur your desktop.
Just to be clear: I never tried to argue for always setting max_bpc to something > 8 (or setting it at all). That decision should be done kernel side only, imho. If I'm not completely mistaken that's the way intel does it already.
You are completly free to loop over your active planes in your kernel driver and check format/colorspace and set the max bpc according to that.
No. Changing bits per colour requires a modeset (and so potentially a few seconds of blank screen when it occurs). From the kernels point of view planes may come and go frequently. It would be wrong be wrong for a kernel to force a modeset because the 10-bit video layer disappeared for a frame when switching video in a video playlist, or using trickplay. Userspace has a much higher level view of what is needed and so can make a better decision.
No. Changing bits per colour requires a modeset (and so potentially a few seconds of blank screen when it occurs). From the kernels point of view planes may come and go frequently.
Implementation detail. It would be the only thing which is correct (also to your previous comments, i.e. don't output 10 bit if only 8 bit content). You should have a hook for format or color-encoding (plane-property-) change anyway.
As I said previously and was my main concern: Only because a certain device can decode a 10bit video doesn't necessarly say it can ouput 10 bit - it's just a heuristic which might be true in most cases. Userspace does not have enough information to make that decision finally.
@knaerzche I'm not sure I really understand you. If we set max_bpc to eg 12 the kernel driver is still free to reject that or choose a lower bit depth - it's only an upper limit
Maybe it would be beneficial to add an (advanced) setting to disable touching max_bpc in kodi - then you could control it statically from userspace/system init/...
I'm fully aware what the meaning of max_bpc is and I'm not saying that there would be a way to check this driver side and ignore it if not supported and so on. Pls check https://github.com/torvalds/linux/blob/master/include/drm/drm_connector.h#L823-L826 - it is the max bpc supported by the connector (e.g. from edid). If you really want to do it this way - you should swich to max_requested_bpc https://github.com/torvalds/linux/blob/master/include/drm/drm_connector.h#L816-L820
It is, btw, even more wrong to set BT2020 colorspace (again from video content) without even checking it is suported by the connector and hoping it will be rejected/ignored somewhere else.
@knaerzche the "max bpc" connector property sets max_requested_bpc in connector state - see https://github.com/torvalds/linux/blob/master/drivers/gpu/drm/drm_connector.c#L1233-L1238 , https://github.com/torvalds/linux/blob/master/drivers/gpu/drm/drm_connector.c#L2218-L2250 and https://github.com/torvalds/linux/blob/master/drivers/gpu/drm/drm_atomic_uapi.c#L800-L801
I agree colorspace should only be set if the sink supports it, this can be added after the edid PR https://github.com/xbmc/xbmc/pull/19141 has been merged.
Well, you really should try to get away from: It works for me, so it must be correct - which really smells like typical vendor behavior, e.g. "Let's set it to 12 for everything above 8 (no matter what the content is), because it fits best for my HW" Please re-read the documentation of max_bpc and ask yourself if what you are doing here is in line with it (hint: it's a limitation) - also the way you are trying to detect it. I'm out of arguments.
