On 2018-05-16 01:07 PM, Michel Dänzer wrote:
> 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!
>
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)
>
>
