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

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

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