[PATCH xf86-video-amdgpu 01/13] Add color management properties to driver-private CRTC object

[Michel Dänzer]

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