On 2018-02-27 05:16 PM, Harry Wentland wrote: > On 2018-02-27 11:08 AM, Michel Dänzer wrote: >> On 2018-02-27 04:54 PM, Leo Li wrote: >>> On 2018-02-27 05:34 AM, Michel Dänzer wrote: >>>> On 2018-02-26 09:15 PM, Harry Wentland wrote: >>>>> From: "Leo (Sunpeng) Li" <sunpeng.li at amd.com> >>>>> >>>>> Non-legacy LUT size should reflect hw capability. Change size from 256 >>>>> to 4096. >>>>> >>>>> However, X doesn't seem to play with legacy LUTs of such size. >>>>> Therefore, check for legacy lut when updating DC states, and update >>>>> accordingly. >>>>> >>>>> v2: Use a macro for the maximum drm LUT value. >>>>> >>>>> Signed-off-by: Leo (Sunpeng) Li <sunpeng.li at amd.com> >>>>> Reviewed-by: Harry Wentland <Harry.Wentland at amd.com> >>>>> --- >>>>>  drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 2 +- >>>>>  drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h | 4 +- >>>>>  .../drm/amd/display/amdgpu_dm/amdgpu_dm_color.c   | 77 >>>>> ++++++++++++++++------ >>>>>  3 files changed, 61 insertions(+), 22 deletions(-) >>>>> >>>>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c >>>>> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c >>>>> index bb9405daa087..f782518fc424 100644 >>>>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c >>>>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c >>>>> @@ -3228,7 +3228,7 @@ static int amdgpu_dm_crtc_init(struct >>>>> amdgpu_display_manager *dm, >>>>>      dm->adev->mode_info.crtcs[crtc_index] = acrtc; >>>>>      drm_crtc_enable_color_mgmt(&acrtc->base, MAX_COLOR_LUT_ENTRIES, >>>>>                     true, MAX_COLOR_LUT_ENTRIES); >>>>> -   drm_mode_crtc_set_gamma_size(&acrtc->base, MAX_COLOR_LUT_ENTRIES); >>>>> +   drm_mode_crtc_set_gamma_size(&acrtc->base, >>>>> MAX_COLOR_LEGACY_LUT_ENTRIES); >>>> >>>> This means userspace can't set gamma ramps with more than 256 entries, >>>> since drm_mode_gamma_set_ioctl returns an error if the sizes don't >>>> match. We should just fix userspace which tries to use the wrong size. >>> >>> It seems X hard-codes 256 entries for the legacy lut, ignoring what the >>> kernel requests: >>> >>> xf86CrtcCreate in xserver/hw/xfree86/modes/xf86Crtc.c (~line 120): >>> >>> /* Preallocate gamma at a sensible size. */ >>> crtc->gamma_size = 256; >>> >>> >>> I confirmed this as well via a printk on the user LUT size in >>> drm_mode_gamma_set_ioctl (after reverting to 4096 gamma size, of course). >>> >>> I'm not sure if modifying that line in X will be straightforward. >> >> It could be overridden by the DDX driver. However, there is the issue of >> using an older DDX driver with a newer kernel. >> >> >>> It's worth noting that drm_mode_crtc_set_gamma_size() only sets the legacy >>> gamma size. Non-legacy gamma (de/regamma) sizes are reported via a mode >>> property, and does not conflict with legacy size. Therefore, reporting a >>> different size for legacy shouldn't be an issue, AFAIK; as long as we >>> check the DRM lut size and update DC accordingly, which we're doing. >> >> Makes sense, but it means that gamma will not work for 30-bit colour in >> Xorg until the DDX driver uses the new gamma mechanism, which might >> require switching to the atomic KMS API as well. Is that okay? >> > > That's still current behavior, so this patch is not changing anything there. We could increase it but how would we deal with old DDX/X that assumes 256? Would it make sense instead to teach X to set the new gamma_lut property? Maybe. Can it be used without using the atomic KMS UAPI in general? -- Earthling Michel Dänzer | http://www.amd.com Libre software enthusiast | Mesa and X developer
