| Message ID | 1462369234-27807-1-git-send-email-lionel.g.landwerlin@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. > + intel_color_load_luts(crtc->state); > + } > } > > for_each_crtc_in_state(state, crtc, old_crtc_state, i) { > -- > 2.8.0.rc3.226.g39d4020
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
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 >
On Fri, May 06, 2016 at 01:52:58PM +0300, Ville Syrjälä wrote: > 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. I really need to unlazy and type in my generic vblank workers ... -Daniel
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); + intel_color_load_luts(crtc->state); + } } for_each_crtc_in_state(state, crtc, old_crtc_state, i) {
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(+)