Re: [PATCH] drm/i915: add missing condition for committing planes on crtc

Lionel Landwerlin
2016-05-05T14:07:10Z

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@linux.intel.com>
>> Cc: Jani Nikula <jani.nikula@intel.com>
>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@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