[Intel-gfx] [PATCH] drm/i915: add missing condition for committing planes on crtc

[Lionel Landwerlin]

On 04/05/16 15:30, Ville Syrjälä wrote:
> On Wed, May 04, 2016 at 02:40:34PM +0100, Lionel Landwerlin wrote:
>> We are currently missing the color management update condition to
>> commit planes on crtc.
>>
>> v2: add comment about moving the commit of color management registers
>>      to an async worker
>>
>> v3: Commit color management register right after vblank
>>
>> Fixes: 20a34e78f0d7 (drm/i915: Update color management during vblank evasion.)
>> Cc: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
>> Cc: Jani Nikula <jani.nikula at intel.com>
>> Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
>> Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
>> Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin at intel.com>
>> ---
>>   drivers/gpu/drm/i915/intel_display.c | 5 +++++
>>   1 file changed, 5 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 45c218d..c6acfe5 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -13688,6 +13688,11 @@ static int intel_atomic_commit(struct drm_device *dev,
>>   
>>   		if (dev_priv->display.optimize_watermarks)
>>   			dev_priv->display.optimize_watermarks(intel_cstate);
>> +
>> +		if (crtc->state->color_mgmt_changed) {
>> +			intel_color_set_csc(crtc->state);
> As I said earlier, csc shouldn't be here, at least on pch
> platforms. And someone should actually double check whether
> vlv/chv have double buffered csc registers or not. Oh and
> with these frankensocs the double buffering scheme used
> (if any) might be totally crazy, like it is for the pipe B
> primary plane scaler on chv. A fact which the spec fails
> to explain IIRC. So I'd recommend poking at the hardware
> to figure out how it actually works.

Where would you put this for pch platforms?
If this patch is wrong the surely the content of intel_begin_crtc_commit 
is too right?

I'm also struggling to understand why the double buffering of the CSC 
registers matters.
Most people will want to configure this at the same time they configure 
the gamma/degamma
LUTs to achieve color management and if the LUTs aren't double buffered 
then why is it
relevant for the CSC?

Thanks for your time :)

>
>> +			intel_color_load_luts(crtc->state);
>> +		}
>>   	}
>>   
>>   	for_each_crtc_in_state(state, crtc, old_crtc_state, i) {
>> -- 
>> 2.8.0.rc3.226.g39d4020