On Thu, Oct 10, 2019 at 12:23:05PM -0400, Ilia Mirkin wrote:
On Thu, Oct 10, 2019 at 12:01 PM Sean Paul sean@poorly.run wrote:
+static int vop_crtc_atomic_check(struct drm_crtc *crtc,
struct drm_crtc_state *crtc_state)+{
struct vop *vop = to_vop(crtc);if (vop->lut_regs && crtc_state->color_mgmt_changed &&crtc_state->gamma_lut) {unsigned int len;len = drm_color_lut_size(crtc_state->gamma_lut);if (len != crtc->gamma_size) {DRM_DEBUG_KMS("Invalid LUT size; got %d, expected %d\n",len, crtc->gamma_size);return -EINVAL;}Overflow is avoided in drm_mode_gamma_set_ioctl(), so I don't think you need this function.
But that only applies to the legacy path. Isn't this needed to ensure a gamma blob has the right size?
Yeah, good point, we check the element size in the atomic path, but not the max size. I haven't looked at enough color lut stuff to have an opinion whether this check would be useful in a helper function or not, something to consider, I suppose.
Some implementations support multiple sizes (e.g. 256 and 1024) but not anything in between. It would be difficult to expose this generically, I would imagine. The 256 size is kind of special, since basically all legacy usage assumes that 256 is the one true quantity of LUT entries...
What we do currently in i915 is: crtc->gamma_size = 256 GAMMA_LUT_SIZE = platform specific (256, 129, 257, 2^10, or 2^18+1 (lol)) DEGAMMA_LUT_SIZE = platform specific (0, 33, 65, or 2^10)
i915 will accept: - gamma lut of size 256, iff ctm==NULL and degamma==NULL (the so called "legacy gamma" mode) - (de)gamma_lut of size (DE)GAMMA_LUT_SIZE if it passes the checks done by drm_color_lut_check()
Ie. just one or two gamma modes per platform is exposed. And that's about all we can do with the current uapi even though our hardware supports many more modes.
The resulting precision, interpolation vs. truncation behaviour, and handling of out of gamut values are all totally unspecified and userspace just has to make a guess.
We also cheat with the 2^10 sized LUTs a bit due to the hw sharing the same LUT for gamma and degamma, and so if you enable both at the same time we throw away every second entry and each stage only gets a 2^9 entry LUT in the end.
Oh and for the 2^18+1 monstrosity we cheat even more and throw away ~99.8% of the entries :(
This here was my idea for extending the uapi so that we could expose the full hw capabilities and let userspace decide which mode suits it best without having to guess what it'll get: https://github.com/vsyrjala/linux/commits/gamma_mode_prop
Maybe in a few years I'll find time to get back to it...