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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Thu, May 05, 2016 at 03:04:54PM +0100, Lionel Landwerlin wrote:
> 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?

Where you tried to put it originally.

> If this patch is wrong the surely the content of intel_begin_crtc_commit 
> is too right?

Nope, looks correct to me. Everything in there is double buffered
AFAICS.

> 
> 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?

We want the update to be atomic, for every piece of hardware affecting
the ouptut of the pipe. Otherwise it's going to look like crap.

For registers that are double buffered on vblank the atomicity can be
achieved by arming the updates for everything right after evading
the vblank.

For single buffered registers we'd need to write the registers during
vblank. But since we suck and can't do that, simply writing them
somewhere is the best we can do.

> 
> 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
> 

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx