On 2018-05-16 01:07 PM, Michel Dänzer wrote:
On 2018-05-03 08:31 PM, sunpeng...@amd.com wrote:
From: "Leo (Sunpeng) Li" <sunpeng...@amd.com>

Non-legacy color management consists of 3 properties on the CRTC:
Degamma LUT, Color Transformation Matrix (CTM), and Gamma LUT.

Add these properties to the driver-private CRTC, and initialize them
when the CRTC is initialized. These values are refered to as "staged"
values. They exist in the DDX driver, but require a "push" to DRM in
order to be realized in hardware.

Also add a destructor for the driver-private CRTC, which cleans up the
non-legacy properties.

Signed-off-by: Leo (Sunpeng) Li <sunpeng...@amd.com>

Note that while I have some cosmetic feedback on this patch (some of
which may also apply to others), in general the code in this series is
cleanly formatted and well documented, thanks!


Thanks for the review! Will fix all the cosmetic issues.


diff --git a/src/drmmode_display.c b/src/drmmode_display.c
index 49284c6..0ffc6ad 100644
--- a/src/drmmode_display.c
+++ b/src/drmmode_display.c
@@ -747,6 +747,83 @@ drmmode_crtc_scanout_update(xf86CrtcPtr crtc, 
DisplayModePtr mode,
[...]
+char *CM_PROP_NAMES[] = {

This identifier should be lowercase, since it's not a macro.


+               if (get_cm_enum_from_str(drm_prop->name) == prop_id){

Missing space between ) and {.

Also, no empty lines after { or before } please.


@@ -1353,6 +1446,18 @@ drmmode_crtc_init(ScrnInfoPtr pScrn, drmmode_ptr 
drmmode, drmModeResPtr mode_res
        crtc->driver_private = drmmode_crtc;
        drmmode_crtc_hw_id(crtc);
+ drmmode_crtc->gamma_lut_size =
+               (uint32_t)get_drm_cm_prop_value(crtc, CM_GAMMA_LUT_SIZE);
+       drmmode_crtc->degamma_lut_size =
+               (uint32_t)get_drm_cm_prop_value(crtc, CM_DEGAMMA_LUT_SIZE);

Calling drmModeObjectGetProperties and iterating over the properties
twice seems a bit inefficient. Can you combine this to one
drmModeObjectGetProperties call, then iterating over the properties
once, until drmmode_crtc->(de)gamma_lut_size are both non-0?


Yep, it seems this is the only function that's calling it anyways.

Leo


+       drmmode_crtc->ctm = calloc(1, sizeof(*drmmode_crtc->ctm));
+       if (drmmode_crtc->ctm == NULL)

        if (!drmmode_crtc->ctm)


_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Reply via email to