On 2018-05-03 08:31 PM, sunpeng.li at amd.com wrote:
> From: "Leo (Sunpeng) Li" <sunpeng.li at 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.li at 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!
> 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?
> + drmmode_crtc->ctm = calloc(1, sizeof(*drmmode_crtc->ctm));
> + if (drmmode_crtc->ctm == NULL)
if (!drmmode_crtc->ctm)
--
Earthling Michel Dänzer | http://www.amd.com
Libre software enthusiast | Mesa and X developer
