Re: [PATCH xf86-video-amdgpu 0/7] Enabling Color Management - Round 3

From: Leo Li <sunpeng.li-5C7GfCeVMHo@public.gmane.org>
To: "Michel Dänzer" <michel-otUistvHUpPR7s880joybQ@public.gmane.org>
Cc: harry.wentland-5C7GfCeVMHo@public.gmane.org,
	amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
Subject: Re: [PATCH xf86-video-amdgpu 0/7] Enabling Color Management - Round 3
Date: Thu, 14 Jun 2018 15:49:00 -0400	[thread overview]
Message-ID: <a5a043dd-cd40-33bc-1e1d-e3f8980cb69d@amd.com> (raw)
In-Reply-To: <afdb23a3-8722-7928-a612-291b42e7f260-otUistvHUpPR7s880joybQ@public.gmane.org>

On 2018-06-14 12:57 PM, Michel Dänzer wrote:
> 
> Hi Leo,
> 
> 
> sorry for the delay.
> 

Appreciate the review, it's not a small change by any means :)

> 
> On 2018-06-01 06:03 PM, sunpeng.li@amd.com wrote:
>> From: "Leo (Sunpeng) Li" <sunpeng.li@amd.com>
>>
>> This ended up being different enough from v2 to warrant a new patchset. Per
>> Michel's suggestions, there have been various optimizations and cleanups.
>> Here's what's changed:
>>
>> * Cache DRM color management property IDs at pre-init,
>>      * instead of querying DRM each time we need to modify color properties.
>>
>> * Remove drmmode_update_cm_props().
>>      * Update color properties in drmmode_output_get_property() instead.
>>      * This also makes old calls to update_cm_props redundant.
>>
>> * Get rid of fake CRTCs.
>>      * Previously, we were allocating a fake CRTC to configure color props on
>>        outputs that don't have a CRTC.
>>      * Instead, rr_configure_and_change_cm_property() can be easily modified to
>>        accept NULL CRTCs.
> 
> Is it currently ever the case in the hardware / kernel, or expected to
> ever be, that colour management is supported for some but not all CRTCs
> of a GPU?
> 

This was more of an effort to align with what DRM allows, which is
per-CRTC cm support and LUT sizes. I'm not aware of any current or
future hardware where this is the case though. It was a relatively
simple thing to implement, so I thought, might as well.

> If not, the LUT sizes could be tracked once instead of per CRTC, and at
> least the (DE)GAMMA_LUT_SIZE properties could always return the proper
> values, even if the output currently isn't associated with a CRTC.
>

My original take was that it's best to support what the framework
allows. But it does sound like this would have more utility to the
client, and simplify the code as well.

> 
> Other than that, I'm going to send some minor feedback on patches 2 & 3.
> If you prefer, I could fix up those and other cosmetic issues before
> pushing the patches.
> 

I can incorporate those along with the edit above.

Thanks!
Leo

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