| Message ID | 1476278901-15750-7-git-send-email-maarten.lankhorst@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Oct 12, 2016 at 03:28:19PM +0200, Maarten Lankhorst wrote: > There's no need to keep a duplicate skl_pipe_wm around any more, > everything can be discovered from crtc_state, which we pass around > correctly now even in case of plane disable. You might want to add some clarification that intel(crtc->state)->wm.skl.optimal and intel_crtc->wm.active always hold the same value by the time we finally drop our CRTC locks, so there's no need for the duplication on gen9. The reason we have intel_crtc->wm.active in general is because the two-step platforms (ILK-style, VLV-style) need cross-CRTC information during watermark updates (which are potentially racing if multiple CRTC's are updated independently but simultaneously) and the intel_crtc data is protected by a separate wm_mutex. Watermark calculations triggered by one CRTC's update need to know which values are actually active on the hardware (old, intermediate, or final) while another CRTC update is still being processed. The types of CRTC updates that are allowed to race on gen9 don't have the same kind of inter-CRTC data dependency. I think this patch needs some slight rebasing to apply cleanly on top of Lyude's changes that landed yesterday, but if you expand the commit message justification a bit, Reviewed-by: Matt Roper <matthew.d.roper@intel.com> Matt > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > --- > drivers/gpu/drm/i915/intel_display.c | 2 +- > drivers/gpu/drm/i915/intel_drv.h | 1 - > drivers/gpu/drm/i915/intel_pm.c | 11 +++++------ > 3 files changed, 6 insertions(+), 8 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 23d8c72dade3..340861826c46 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -13455,7 +13455,7 @@ static void verify_wm_state(struct drm_crtc *crtc, > return; > > skl_pipe_wm_get_hw_state(crtc, &hw_wm); > - sw_wm = &intel_crtc->wm.active.skl; > + sw_wm = &to_intel_crtc_state(new_state)->wm.skl.optimal; > > skl_ddb_get_hw_state(dev_priv, &hw_ddb); > sw_ddb = &dev_priv->wm.skl_hw.ddb; > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index a176e6cebab3..9f04e26c4365 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -723,7 +723,6 @@ struct intel_crtc { > /* watermarks currently being used */ > union { > struct intel_pipe_wm ilk; > - struct skl_pipe_wm skl; > } active; > > /* allow CxSR on this pipe */ > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > index 05ccd253fd7a..be3dd8cdc7ae 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -3904,9 +3904,9 @@ bool skl_ddb_allocation_overlaps(struct drm_atomic_state *state, > static int skl_update_pipe_wm(struct drm_crtc_state *cstate, > struct skl_ddb_allocation *ddb, /* out */ > struct skl_pipe_wm *pipe_wm, /* out */ > + const struct skl_pipe_wm *old_pipe_wm, > bool *changed /* out */) > { > - struct intel_crtc *intel_crtc = to_intel_crtc(cstate->crtc); > struct intel_crtc_state *intel_cstate = to_intel_crtc_state(cstate); > int ret; > > @@ -3914,7 +3914,7 @@ static int skl_update_pipe_wm(struct drm_crtc_state *cstate, > if (ret) > return ret; > > - if (!memcmp(&intel_crtc->wm.active.skl, pipe_wm, sizeof(*pipe_wm))) > + if (!memcmp(old_pipe_wm, pipe_wm, sizeof(*pipe_wm))) > *changed = false; > else > *changed = true; > @@ -4155,10 +4155,12 @@ skl_compute_wm(struct drm_atomic_state *state) > for_each_crtc_in_state(state, crtc, cstate, i) { > struct intel_crtc_state *intel_cstate = > to_intel_crtc_state(cstate); > + const struct skl_pipe_wm *old_pipe_wm = > + &to_intel_crtc_state(crtc->state)->wm.skl.optimal; > > pipe_wm = &intel_cstate->wm.skl.optimal; > ret = skl_update_pipe_wm(cstate, &results->ddb, pipe_wm, > - &changed); > + old_pipe_wm, &changed); > if (ret) > return ret; > > @@ -4203,8 +4205,6 @@ static void skl_update_wm(struct drm_crtc *crtc) > if ((results->dirty_pipes & drm_crtc_mask(crtc)) == 0) > return; > > - intel_crtc->wm.active.skl = *pipe_wm; > - > mutex_lock(&dev_priv->wm.wm_mutex); > > /* > @@ -4371,7 +4371,6 @@ void skl_wm_get_hw_state(struct drm_device *dev) > cstate = to_intel_crtc_state(crtc->state); > > skl_pipe_wm_get_hw_state(crtc, &cstate->wm.skl.optimal); > - intel_crtc->wm.active.skl = cstate->wm.skl.optimal; > > if (!intel_crtc->active) > hw->dirty_pipes |= drm_crtc_mask(crtc); > -- > 2.7.4 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Em Qua, 2016-10-12 às 15:28 +0200, Maarten Lankhorst escreveu: > There's no need to keep a duplicate skl_pipe_wm around any more, > everything can be discovered from crtc_state, which we pass around > correctly now even in case of plane disable. > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > --- > drivers/gpu/drm/i915/intel_display.c | 2 +- > drivers/gpu/drm/i915/intel_drv.h | 1 - > drivers/gpu/drm/i915/intel_pm.c | 11 +++++------ > 3 files changed, 6 insertions(+), 8 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c > b/drivers/gpu/drm/i915/intel_display.c > index 23d8c72dade3..340861826c46 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -13455,7 +13455,7 @@ static void verify_wm_state(struct drm_crtc > *crtc, > return; > > skl_pipe_wm_get_hw_state(crtc, &hw_wm); > - sw_wm = &intel_crtc->wm.active.skl; > + sw_wm = &to_intel_crtc_state(new_state)->wm.skl.optimal; > > skl_ddb_get_hw_state(dev_priv, &hw_ddb); > sw_ddb = &dev_priv->wm.skl_hw.ddb; > diff --git a/drivers/gpu/drm/i915/intel_drv.h > b/drivers/gpu/drm/i915/intel_drv.h > index a176e6cebab3..9f04e26c4365 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -723,7 +723,6 @@ struct intel_crtc { > /* watermarks currently being used */ > union { > struct intel_pipe_wm ilk; > - struct skl_pipe_wm skl; > } active; > > /* allow CxSR on this pipe */ > diff --git a/drivers/gpu/drm/i915/intel_pm.c > b/drivers/gpu/drm/i915/intel_pm.c > index 05ccd253fd7a..be3dd8cdc7ae 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -3904,9 +3904,9 @@ bool skl_ddb_allocation_overlaps(struct > drm_atomic_state *state, > static int skl_update_pipe_wm(struct drm_crtc_state *cstate, > struct skl_ddb_allocation *ddb, /* out > */ > struct skl_pipe_wm *pipe_wm, /* out */ > + const struct skl_pipe_wm *old_pipe_wm, > bool *changed /* out */) Bikeshed: this patch adds an "in" parameter in the middle of the "out" parameters. That's kinda ugly IMHO. With that maybe fixed: Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > { > - struct intel_crtc *intel_crtc = to_intel_crtc(cstate->crtc); > struct intel_crtc_state *intel_cstate = > to_intel_crtc_state(cstate); > int ret; > > @@ -3914,7 +3914,7 @@ static int skl_update_pipe_wm(struct > drm_crtc_state *cstate, > if (ret) > return ret; > > - if (!memcmp(&intel_crtc->wm.active.skl, pipe_wm, > sizeof(*pipe_wm))) > + if (!memcmp(old_pipe_wm, pipe_wm, sizeof(*pipe_wm))) > *changed = false; > else > *changed = true; > @@ -4155,10 +4155,12 @@ skl_compute_wm(struct drm_atomic_state > *state) > for_each_crtc_in_state(state, crtc, cstate, i) { > struct intel_crtc_state *intel_cstate = > to_intel_crtc_state(cstate); > + const struct skl_pipe_wm *old_pipe_wm = > + &to_intel_crtc_state(crtc->state)- > >wm.skl.optimal; > > pipe_wm = &intel_cstate->wm.skl.optimal; > ret = skl_update_pipe_wm(cstate, &results->ddb, > pipe_wm, > - &changed); > + old_pipe_wm, &changed); > if (ret) > return ret; > > @@ -4203,8 +4205,6 @@ static void skl_update_wm(struct drm_crtc > *crtc) > if ((results->dirty_pipes & drm_crtc_mask(crtc)) == 0) > return; > > - intel_crtc->wm.active.skl = *pipe_wm; > - > mutex_lock(&dev_priv->wm.wm_mutex); > > /* > @@ -4371,7 +4371,6 @@ void skl_wm_get_hw_state(struct drm_device > *dev) > cstate = to_intel_crtc_state(crtc->state); > > skl_pipe_wm_get_hw_state(crtc, &cstate- > >wm.skl.optimal); > - intel_crtc->wm.active.skl = cstate->wm.skl.optimal; > > if (!intel_crtc->active) > hw->dirty_pipes |= drm_crtc_mask(crtc);
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 23d8c72dade3..340861826c46 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -13455,7 +13455,7 @@ static void verify_wm_state(struct drm_crtc *crtc, return; skl_pipe_wm_get_hw_state(crtc, &hw_wm); - sw_wm = &intel_crtc->wm.active.skl; + sw_wm = &to_intel_crtc_state(new_state)->wm.skl.optimal; skl_ddb_get_hw_state(dev_priv, &hw_ddb); sw_ddb = &dev_priv->wm.skl_hw.ddb; diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index a176e6cebab3..9f04e26c4365 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -723,7 +723,6 @@ struct intel_crtc { /* watermarks currently being used */ union { struct intel_pipe_wm ilk; - struct skl_pipe_wm skl; } active; /* allow CxSR on this pipe */ diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index 05ccd253fd7a..be3dd8cdc7ae 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -3904,9 +3904,9 @@ bool skl_ddb_allocation_overlaps(struct drm_atomic_state *state, static int skl_update_pipe_wm(struct drm_crtc_state *cstate, struct skl_ddb_allocation *ddb, /* out */ struct skl_pipe_wm *pipe_wm, /* out */ + const struct skl_pipe_wm *old_pipe_wm, bool *changed /* out */) { - struct intel_crtc *intel_crtc = to_intel_crtc(cstate->crtc); struct intel_crtc_state *intel_cstate = to_intel_crtc_state(cstate); int ret; @@ -3914,7 +3914,7 @@ static int skl_update_pipe_wm(struct drm_crtc_state *cstate, if (ret) return ret; - if (!memcmp(&intel_crtc->wm.active.skl, pipe_wm, sizeof(*pipe_wm))) + if (!memcmp(old_pipe_wm, pipe_wm, sizeof(*pipe_wm))) *changed = false; else *changed = true; @@ -4155,10 +4155,12 @@ skl_compute_wm(struct drm_atomic_state *state) for_each_crtc_in_state(state, crtc, cstate, i) { struct intel_crtc_state *intel_cstate = to_intel_crtc_state(cstate); + const struct skl_pipe_wm *old_pipe_wm = + &to_intel_crtc_state(crtc->state)->wm.skl.optimal; pipe_wm = &intel_cstate->wm.skl.optimal; ret = skl_update_pipe_wm(cstate, &results->ddb, pipe_wm, - &changed); + old_pipe_wm, &changed); if (ret) return ret; @@ -4203,8 +4205,6 @@ static void skl_update_wm(struct drm_crtc *crtc) if ((results->dirty_pipes & drm_crtc_mask(crtc)) == 0) return; - intel_crtc->wm.active.skl = *pipe_wm; - mutex_lock(&dev_priv->wm.wm_mutex); /* @@ -4371,7 +4371,6 @@ void skl_wm_get_hw_state(struct drm_device *dev) cstate = to_intel_crtc_state(crtc->state); skl_pipe_wm_get_hw_state(crtc, &cstate->wm.skl.optimal); - intel_crtc->wm.active.skl = cstate->wm.skl.optimal; if (!intel_crtc->active) hw->dirty_pipes |= drm_crtc_mask(crtc);
There's no need to keep a duplicate skl_pipe_wm around any more, everything can be discovered from crtc_state, which we pass around correctly now even in case of plane disable. Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> --- drivers/gpu/drm/i915/intel_display.c | 2 +- drivers/gpu/drm/i915/intel_drv.h | 1 - drivers/gpu/drm/i915/intel_pm.c | 11 +++++------ 3 files changed, 6 insertions(+), 8 deletions(-)