Re: [PATCH xf86-video-amdgpu 00/13] Enabling Color Management - Round 2

[Leo Li <sunpeng.li-5C7GfCeVMHo@public.gmane.org>]

Ping :)

Leo

On 2018-05-03 02:31 PM, sunpeng.li@amd.com wrote:
> From: "Leo (Sunpeng) Li" <sunpeng.li@amd.com>
> 
> 
> This patchset ended up looking quite different from the first. To address some
> fundamental issues, the design had to be reworked.
> 
> Things gathered from previous review:
> 
> 1. User client should not have to handle DRM blob objects. That should be the
>     job of the DDX driver.
> 
> 2. Since legacy gamma sets the same properties within DRM as non-legacy gamma,
>     the previous implementation created conflicts when setting both.
> 
>      * We should at least support this use-case: Nightlight enabled (uses legacy
>        gamma), with monitor correction enabled (non-legacy gamma)
> 
> 3. Since color management properties are attached to the CRTC, the previous
>     revision has the properties hooked onto the CRTC life-cycle, not the output
>     life-cycle. This is problematic, since clients expect properties on an
>     output to stay consistent throughout its life.
> 
> 4. Although not mentioned during review, the color properties did not persist
>     across certain events, such as DPMS state changes or hotplugs.
> 
> 
> To address the above, the following was done:
> 
> 1. XRandR allows setting of array-like properties. This is used to directly
>     pass LUT/CTM data from the client to the DDX driver.
> 
> 2. Legacy and non-legacy gamma LUTs are now merged via composition. This will
>     allow both to be in effect, while only programming one LUT in kernel driver.
> 
> 3. The three color management properties (Degamma LUT, Color Transform Matrix
>     (CTM), and Gamma LUT) are hard-coded into the DDX driver, to be listed (as
>     disabled) regardless of whether a CRTC is attached on the output, or whether
>     the kernel driver supports it.
> 
>      * If kernel driver does not support color management, the properties will
>        remain disabled. A `xrandr --set` will then error.
> 
> 4. Color properties are now *staged* inside the driver-private CRTC object.
>     This allows us to *push* the properties into kernel DRM (and consequently
>     into hardware) whenever there is a need.
> 
>      * Along with staging and pushing, an *update* function is used to notify
>        RandR to update the color properties listed on outputs. This can be used
>        when `xrandr --auto` enables a CRTC on an output, and the output need to
>        reflect the CRTC's color properties.
> 
> 
> However, there are some things being done that aren't quite nice, to which I
> don't yet have a solution. Any thoughts and suggestions are welcome:
> 
> * When using libXrandr to set the CTM property, 16bit format is used. Ideally
>    we should use 64bit format, since the CTM consists of 9x64bit fixed-point
>    values. However, it isn't recognized by XRRChangeOutputProperty as a valid
>    format. 32 bit could work, since the CTM values are S31.32 fixed-point.
>    However, using this format corrupts the data once it gets to xserver. On
>    first glance, it may be the cast to long within XRRChangeOutputProperty, in
>    addition to compiling 64 bit (shouldn't it use int32_t instead?).
> 
> * Since LUTs can be quite long, the output of `xrandr --prop` isn't very nice.
> 
> * Setting these properties through the xrandr app isn't supported without
>    modifications to the app, due to the length of these properties. However,
>    setting through libXrandr works.
> 
> Support on the xrandr app side can come as a seperate patch set. For now,
> testing the new API can be done via libXrandr. I've made a sample application
> here: git://people.freedesktop.org/~hwentland/color-demo-app
> Clone, make, and it's ready to go.
> 
> 
> Leo (Sunpeng) Li (13):
>    Add color management properties to driver-private CRTC object
>    Push color properties to kernel DRM on CRTC init
>    List disabled color properties on RandR outputs without a CRTC
>    Use CRTC's color properties if output has a CRTC attached
>    Enable setting of color properties though RandR
>    Compose non-legacy with legacy gamma LUT before pushing to kernel DRM
>    Also compose LUT when setting legacy gamma.
>    Set driver-private CRTC's dpms mode on disable
>    Move drmmode_do_crtc_dpms
>    Push staged color properties when DPMS state toggles On
>    Push staged color properties on output detect
>    Update color properties on modeset major
>    Refactor pushing color management properties into a function
> 
>   src/drmmode_display.c | 878 ++++++++++++++++++++++++++++++++++++++++++++++----
>   src/drmmode_display.h |   8 +
>   2 files changed, 824 insertions(+), 62 deletions(-)
> 
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx