| Message ID | 1476278901-15750-6-git-send-email-maarten.lankhorst@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Oct 12, 2016 at 03:28:18PM +0200, Maarten Lankhorst wrote: > Allow the driver to write watermarks during atomic evasion. > This will make it possible to write the watermarks in a cleaner > way on gen9+. > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > --- > drivers/gpu/drm/i915/i915_drv.h | 6 ++++-- > drivers/gpu/drm/i915/intel_display.c | 18 ++++++++---------- > drivers/gpu/drm/i915/intel_pm.c | 19 +++++++++++++++++-- > 3 files changed, 29 insertions(+), 14 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index f65ccf9b0bea..09588c58148f 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -484,6 +484,7 @@ struct sdvo_device_mapping { > > struct intel_connector; > struct intel_encoder; > +struct intel_atomic_state; > struct intel_crtc_state; > struct intel_initial_plane_config; > struct intel_crtc; > @@ -497,8 +498,9 @@ struct drm_i915_display_funcs { > int (*compute_intermediate_wm)(struct drm_device *dev, > struct intel_crtc *intel_crtc, > struct intel_crtc_state *newstate); > - void (*initial_watermarks)(struct intel_crtc_state *cstate); > - void (*optimize_watermarks)(struct intel_crtc_state *cstate); > + void (*initial_watermarks)(struct intel_atomic_state *state, struct intel_crtc_state *cstate); > + void (*atomic_evade_watermarks)(struct intel_atomic_state *state, struct intel_crtc_state *cstate); > + void (*optimize_watermarks)(struct intel_atomic_state *state, struct intel_crtc_state *cstate); initial_watermarks() and optimize_watermarks() are currently only used on ILK (and possibly by in-development VLV/CHV patches that Ville is working on?). As far as I can see, the top-level state that we add as a parameter here doesn't actually get used in the implementations. Are you adding it to just make them more similar to the signature of the new atomic_evade_watermarks vfunc or did you have something else in mind? I'd also suggest adding a brief comment to your new skl_evade_crtc_wm() function that indicates that nearly all of the gen9 watermark values are per-plane values that get written as part of the general plane update in skylake_update_primary_plane and/or skl_update_plane. Given that those two functions are located in other files that may help clarify to future developers why this function appears so trivial. Matt > int (*compute_global_watermarks)(struct drm_atomic_state *state); > void (*update_wm)(struct drm_crtc *crtc); > int (*modeset_calc_cdclk)(struct drm_atomic_state *state); > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 55f8ec8c76ae..23d8c72dade3 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -5160,7 +5160,7 @@ static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state) > * us to. > */ > if (dev_priv->display.initial_watermarks != NULL) > - dev_priv->display.initial_watermarks(pipe_config); > + dev_priv->display.initial_watermarks(to_intel_atomic_state(old_state), pipe_config); > else if (pipe_config->update_wm_pre) > intel_update_watermarks(&crtc->base); > } > @@ -5374,7 +5374,7 @@ static void ironlake_crtc_enable(struct intel_crtc_state *pipe_config, > intel_color_load_luts(&pipe_config->base); > > if (dev_priv->display.initial_watermarks != NULL) > - dev_priv->display.initial_watermarks(intel_crtc->config); > + dev_priv->display.initial_watermarks(to_intel_atomic_state(old_state), intel_crtc->config); > intel_enable_pipe(intel_crtc); > > if (intel_crtc->config->has_pch_encoder) > @@ -5480,7 +5480,7 @@ static void haswell_crtc_enable(struct intel_crtc_state *pipe_config, > intel_ddi_enable_transcoder_func(crtc); > > if (dev_priv->display.initial_watermarks != NULL) > - dev_priv->display.initial_watermarks(pipe_config); > + dev_priv->display.initial_watermarks(to_intel_atomic_state(old_state), pipe_config); > else > intel_update_watermarks(crtc); > > @@ -14503,7 +14503,7 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state) > intel_cstate = to_intel_crtc_state(crtc->state); > > if (dev_priv->display.optimize_watermarks) > - dev_priv->display.optimize_watermarks(intel_cstate); > + dev_priv->display.optimize_watermarks(intel_state, intel_cstate); > } > > for_each_crtc_in_state(state, crtc, old_crtc_state, i) { > @@ -14908,7 +14908,6 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc, > struct intel_crtc_state *old_intel_state = > to_intel_crtc_state(old_crtc_state); > bool modeset = needs_modeset(crtc->state); > - enum pipe pipe = intel_crtc->pipe; > > /* Perform vblank evasion around commit operation */ > intel_pipe_update_start(intel_crtc); > @@ -14923,12 +14922,11 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc, > > if (intel_cstate->update_pipe) > intel_update_pipe_config(intel_crtc, old_intel_state); > - else if (INTEL_GEN(dev_priv) >= 9) { > + else if (INTEL_GEN(dev_priv) >= 9) > skl_detach_scalers(intel_crtc); > > - I915_WRITE(PIPE_WM_LINETIME(pipe), > - intel_cstate->wm.skl.optimal.linetime); > - } > + if (dev_priv->display.atomic_evade_watermarks) > + dev_priv->display.atomic_evade_watermarks(to_intel_atomic_state(old_crtc_state->state), intel_cstate); > } > > static void intel_finish_crtc_commit(struct drm_crtc *crtc, > @@ -16388,7 +16386,7 @@ retry: > struct intel_crtc_state *cs = to_intel_crtc_state(cstate); > > cs->wm.need_postvbl_update = true; > - dev_priv->display.optimize_watermarks(cs); > + dev_priv->display.optimize_watermarks(to_intel_atomic_state(state), cs); > } > > drm_atomic_state_free(state); > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > index 45fb8275abea..05ccd253fd7a 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -4177,6 +4177,18 @@ skl_compute_wm(struct drm_atomic_state *state) > return 0; > } > > +static void skl_evade_crtc_wm(struct intel_atomic_state *state, > + struct intel_crtc_state *cstate) > +{ > + struct intel_crtc *crtc = to_intel_crtc(cstate->base.crtc); > + struct drm_i915_private *dev_priv = to_i915(state->base.dev); > + struct skl_pipe_wm *pipe_wm = &cstate->wm.skl.optimal; > + enum pipe pipe = crtc->pipe; > + > + I915_WRITE(PIPE_WM_LINETIME(pipe), > + pipe_wm->linetime); > +} > + > static void skl_update_wm(struct drm_crtc *crtc) > { > struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > @@ -4270,7 +4282,8 @@ static void ilk_program_watermarks(struct drm_i915_private *dev_priv) > ilk_write_wm_values(dev_priv, &results); > } > > -static void ilk_initial_watermarks(struct intel_crtc_state *cstate) > +static void ilk_initial_watermarks(struct intel_atomic_state *state, > + struct intel_crtc_state *cstate) > { > struct drm_i915_private *dev_priv = to_i915(cstate->base.crtc->dev); > struct intel_crtc *intel_crtc = to_intel_crtc(cstate->base.crtc); > @@ -4281,7 +4294,8 @@ static void ilk_initial_watermarks(struct intel_crtc_state *cstate) > mutex_unlock(&dev_priv->wm.wm_mutex); > } > > -static void ilk_optimize_watermarks(struct intel_crtc_state *cstate) > +static void ilk_optimize_watermarks(struct intel_atomic_state *state, > + struct intel_crtc_state *cstate) > { > struct drm_i915_private *dev_priv = to_i915(cstate->base.crtc->dev); > struct intel_crtc *intel_crtc = to_intel_crtc(cstate->base.crtc); > @@ -7715,6 +7729,7 @@ void intel_init_pm(struct drm_device *dev) > if (INTEL_INFO(dev)->gen >= 9) { > skl_setup_wm_latency(dev); > dev_priv->display.update_wm = skl_update_wm; > + dev_priv->display.atomic_evade_watermarks = skl_evade_crtc_wm; > dev_priv->display.compute_global_watermarks = skl_compute_wm; > } else if (HAS_PCH_SPLIT(dev)) { > ilk_setup_wm_latency(dev); > -- > 2.7.4 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Wed, Oct 19, 2016 at 04:15:02PM -0700, Matt Roper wrote: > On Wed, Oct 12, 2016 at 03:28:18PM +0200, Maarten Lankhorst wrote: > > Allow the driver to write watermarks during atomic evasion. > > This will make it possible to write the watermarks in a cleaner > > way on gen9+. > > > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > --- > > drivers/gpu/drm/i915/i915_drv.h | 6 ++++-- > > drivers/gpu/drm/i915/intel_display.c | 18 ++++++++---------- > > drivers/gpu/drm/i915/intel_pm.c | 19 +++++++++++++++++-- > > 3 files changed, 29 insertions(+), 14 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > > index f65ccf9b0bea..09588c58148f 100644 > > --- a/drivers/gpu/drm/i915/i915_drv.h > > +++ b/drivers/gpu/drm/i915/i915_drv.h > > @@ -484,6 +484,7 @@ struct sdvo_device_mapping { > > > > struct intel_connector; > > struct intel_encoder; > > +struct intel_atomic_state; > > struct intel_crtc_state; > > struct intel_initial_plane_config; > > struct intel_crtc; > > @@ -497,8 +498,9 @@ struct drm_i915_display_funcs { > > int (*compute_intermediate_wm)(struct drm_device *dev, > > struct intel_crtc *intel_crtc, > > struct intel_crtc_state *newstate); > > - void (*initial_watermarks)(struct intel_crtc_state *cstate); > > - void (*optimize_watermarks)(struct intel_crtc_state *cstate); > > + void (*initial_watermarks)(struct intel_atomic_state *state, struct intel_crtc_state *cstate); > > + void (*atomic_evade_watermarks)(struct intel_atomic_state *state, struct intel_crtc_state *cstate); > > + void (*optimize_watermarks)(struct intel_atomic_state *state, struct intel_crtc_state *cstate); > > initial_watermarks() and optimize_watermarks() are currently only used > on ILK (and possibly by in-development VLV/CHV patches that Ville is > working on?). As far as I can see, the top-level state that we add as a > parameter here doesn't actually get used in the implementations. Are > you adding it to just make them more similar to the signature of the new > atomic_evade_watermarks vfunc or did you have something else in mind? For that matter, it doesn't look like it's really used in atomic_evade_watermarks either except to grab dev_priv (which we can obtain in other ways). Matt > > I'd also suggest adding a brief comment to your new skl_evade_crtc_wm() > function that indicates that nearly all of the gen9 watermark values are > per-plane values that get written as part of the general plane update in > skylake_update_primary_plane and/or skl_update_plane. Given that those > two functions are located in other files that may help clarify to future > developers why this function appears so trivial. > > > Matt > > > int (*compute_global_watermarks)(struct drm_atomic_state *state); > > void (*update_wm)(struct drm_crtc *crtc); > > int (*modeset_calc_cdclk)(struct drm_atomic_state *state); > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > index 55f8ec8c76ae..23d8c72dade3 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -5160,7 +5160,7 @@ static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state) > > * us to. > > */ > > if (dev_priv->display.initial_watermarks != NULL) > > - dev_priv->display.initial_watermarks(pipe_config); > > + dev_priv->display.initial_watermarks(to_intel_atomic_state(old_state), pipe_config); > > else if (pipe_config->update_wm_pre) > > intel_update_watermarks(&crtc->base); > > } > > @@ -5374,7 +5374,7 @@ static void ironlake_crtc_enable(struct intel_crtc_state *pipe_config, > > intel_color_load_luts(&pipe_config->base); > > > > if (dev_priv->display.initial_watermarks != NULL) > > - dev_priv->display.initial_watermarks(intel_crtc->config); > > + dev_priv->display.initial_watermarks(to_intel_atomic_state(old_state), intel_crtc->config); > > intel_enable_pipe(intel_crtc); > > > > if (intel_crtc->config->has_pch_encoder) > > @@ -5480,7 +5480,7 @@ static void haswell_crtc_enable(struct intel_crtc_state *pipe_config, > > intel_ddi_enable_transcoder_func(crtc); > > > > if (dev_priv->display.initial_watermarks != NULL) > > - dev_priv->display.initial_watermarks(pipe_config); > > + dev_priv->display.initial_watermarks(to_intel_atomic_state(old_state), pipe_config); > > else > > intel_update_watermarks(crtc); > > > > @@ -14503,7 +14503,7 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state) > > intel_cstate = to_intel_crtc_state(crtc->state); > > > > if (dev_priv->display.optimize_watermarks) > > - dev_priv->display.optimize_watermarks(intel_cstate); > > + dev_priv->display.optimize_watermarks(intel_state, intel_cstate); > > } > > > > for_each_crtc_in_state(state, crtc, old_crtc_state, i) { > > @@ -14908,7 +14908,6 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc, > > struct intel_crtc_state *old_intel_state = > > to_intel_crtc_state(old_crtc_state); > > bool modeset = needs_modeset(crtc->state); > > - enum pipe pipe = intel_crtc->pipe; > > > > /* Perform vblank evasion around commit operation */ > > intel_pipe_update_start(intel_crtc); > > @@ -14923,12 +14922,11 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc, > > > > if (intel_cstate->update_pipe) > > intel_update_pipe_config(intel_crtc, old_intel_state); > > - else if (INTEL_GEN(dev_priv) >= 9) { > > + else if (INTEL_GEN(dev_priv) >= 9) > > skl_detach_scalers(intel_crtc); > > > > - I915_WRITE(PIPE_WM_LINETIME(pipe), > > - intel_cstate->wm.skl.optimal.linetime); > > - } > > + if (dev_priv->display.atomic_evade_watermarks) > > + dev_priv->display.atomic_evade_watermarks(to_intel_atomic_state(old_crtc_state->state), intel_cstate); > > } > > > > static void intel_finish_crtc_commit(struct drm_crtc *crtc, > > @@ -16388,7 +16386,7 @@ retry: > > struct intel_crtc_state *cs = to_intel_crtc_state(cstate); > > > > cs->wm.need_postvbl_update = true; > > - dev_priv->display.optimize_watermarks(cs); > > + dev_priv->display.optimize_watermarks(to_intel_atomic_state(state), cs); > > } > > > > drm_atomic_state_free(state); > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > > index 45fb8275abea..05ccd253fd7a 100644 > > --- a/drivers/gpu/drm/i915/intel_pm.c > > +++ b/drivers/gpu/drm/i915/intel_pm.c > > @@ -4177,6 +4177,18 @@ skl_compute_wm(struct drm_atomic_state *state) > > return 0; > > } > > > > +static void skl_evade_crtc_wm(struct intel_atomic_state *state, > > + struct intel_crtc_state *cstate) > > +{ > > + struct intel_crtc *crtc = to_intel_crtc(cstate->base.crtc); > > + struct drm_i915_private *dev_priv = to_i915(state->base.dev); > > + struct skl_pipe_wm *pipe_wm = &cstate->wm.skl.optimal; > > + enum pipe pipe = crtc->pipe; > > + > > + I915_WRITE(PIPE_WM_LINETIME(pipe), > > + pipe_wm->linetime); > > +} > > + > > static void skl_update_wm(struct drm_crtc *crtc) > > { > > struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > > @@ -4270,7 +4282,8 @@ static void ilk_program_watermarks(struct drm_i915_private *dev_priv) > > ilk_write_wm_values(dev_priv, &results); > > } > > > > -static void ilk_initial_watermarks(struct intel_crtc_state *cstate) > > +static void ilk_initial_watermarks(struct intel_atomic_state *state, > > + struct intel_crtc_state *cstate) > > { > > struct drm_i915_private *dev_priv = to_i915(cstate->base.crtc->dev); > > struct intel_crtc *intel_crtc = to_intel_crtc(cstate->base.crtc); > > @@ -4281,7 +4294,8 @@ static void ilk_initial_watermarks(struct intel_crtc_state *cstate) > > mutex_unlock(&dev_priv->wm.wm_mutex); > > } > > > > -static void ilk_optimize_watermarks(struct intel_crtc_state *cstate) > > +static void ilk_optimize_watermarks(struct intel_atomic_state *state, > > + struct intel_crtc_state *cstate) > > { > > struct drm_i915_private *dev_priv = to_i915(cstate->base.crtc->dev); > > struct intel_crtc *intel_crtc = to_intel_crtc(cstate->base.crtc); > > @@ -7715,6 +7729,7 @@ void intel_init_pm(struct drm_device *dev) > > if (INTEL_INFO(dev)->gen >= 9) { > > skl_setup_wm_latency(dev); > > dev_priv->display.update_wm = skl_update_wm; > > + dev_priv->display.atomic_evade_watermarks = skl_evade_crtc_wm; > > dev_priv->display.compute_global_watermarks = skl_compute_wm; > > } else if (HAS_PCH_SPLIT(dev)) { > > ilk_setup_wm_latency(dev); > > -- > > 2.7.4 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Matt Roper > Graphics Software Engineer > IoTG Platform Enabling & Development > Intel Corporation > (916) 356-2795 > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Hey, Op 20-10-16 om 01:15 schreef Matt Roper: > On Wed, Oct 12, 2016 at 03:28:18PM +0200, Maarten Lankhorst wrote: >> Allow the driver to write watermarks during atomic evasion. >> This will make it possible to write the watermarks in a cleaner >> way on gen9+. >> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >> --- >> drivers/gpu/drm/i915/i915_drv.h | 6 ++++-- >> drivers/gpu/drm/i915/intel_display.c | 18 ++++++++---------- >> drivers/gpu/drm/i915/intel_pm.c | 19 +++++++++++++++++-- >> 3 files changed, 29 insertions(+), 14 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h >> index f65ccf9b0bea..09588c58148f 100644 >> --- a/drivers/gpu/drm/i915/i915_drv.h >> +++ b/drivers/gpu/drm/i915/i915_drv.h >> @@ -484,6 +484,7 @@ struct sdvo_device_mapping { >> >> struct intel_connector; >> struct intel_encoder; >> +struct intel_atomic_state; >> struct intel_crtc_state; >> struct intel_initial_plane_config; >> struct intel_crtc; >> @@ -497,8 +498,9 @@ struct drm_i915_display_funcs { >> int (*compute_intermediate_wm)(struct drm_device *dev, >> struct intel_crtc *intel_crtc, >> struct intel_crtc_state *newstate); >> - void (*initial_watermarks)(struct intel_crtc_state *cstate); >> - void (*optimize_watermarks)(struct intel_crtc_state *cstate); >> + void (*initial_watermarks)(struct intel_atomic_state *state, struct intel_crtc_state *cstate); >> + void (*atomic_evade_watermarks)(struct intel_atomic_state *state, struct intel_crtc_state *cstate); >> + void (*optimize_watermarks)(struct intel_atomic_state *state, struct intel_crtc_state *cstate); > initial_watermarks() and optimize_watermarks() are currently only used > on ILK (and possibly by in-development VLV/CHV patches that Ville is > working on?). As far as I can see, the top-level state that we add as a > parameter here doesn't actually get used in the implementations. Are > you adding it to just make them more similar to the signature of the new > atomic_evade_watermarks vfunc or did you have something else in mind? > > I'd also suggest adding a brief comment to your new skl_evade_crtc_wm() > function that indicates that nearly all of the gen9 watermark values are > per-plane values that get written as part of the general plane update in > skylake_update_primary_plane and/or skl_update_plane. Given that those > two functions are located in other files that may help clarify to future > developers why this function appears so trivial. We don't completely use intel_atomic_state here yet, patch 7 uses it for the ddb allocation and because initial_watermarks ends up calling .atomic_evade_watermarks(). I added intel_atomic_state to all callbacks to keep the function signature identical. It looked better to me to put the function signature in a small change, and then the behavioral change in patch 7 separately, for easier bisection. ~Maarten
Em Qua, 2016-10-12 às 15:28 +0200, Maarten Lankhorst escreveu: > Allow the driver to write watermarks during atomic evasion. > This will make it possible to write the watermarks in a cleaner > way on gen9+. > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > --- > drivers/gpu/drm/i915/i915_drv.h | 6 ++++-- > drivers/gpu/drm/i915/intel_display.c | 18 ++++++++---------- > drivers/gpu/drm/i915/intel_pm.c | 19 +++++++++++++++++-- > 3 files changed, 29 insertions(+), 14 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h > b/drivers/gpu/drm/i915/i915_drv.h > index f65ccf9b0bea..09588c58148f 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -484,6 +484,7 @@ struct sdvo_device_mapping { > > struct intel_connector; > struct intel_encoder; > +struct intel_atomic_state; > struct intel_crtc_state; > struct intel_initial_plane_config; > struct intel_crtc; > @@ -497,8 +498,9 @@ struct drm_i915_display_funcs { > int (*compute_intermediate_wm)(struct drm_device *dev, > struct intel_crtc > *intel_crtc, > struct intel_crtc_state > *newstate); > - void (*initial_watermarks)(struct intel_crtc_state *cstate); > - void (*optimize_watermarks)(struct intel_crtc_state > *cstate); > + void (*initial_watermarks)(struct intel_atomic_state *state, > struct intel_crtc_state *cstate); > + void (*atomic_evade_watermarks)(struct intel_atomic_state > *state, struct intel_crtc_state *cstate); > + void (*optimize_watermarks)(struct intel_atomic_state > *state, struct intel_crtc_state *cstate); Can't we just get intel_atomic_state from intel_crtc_state? Why pass both? > int (*compute_global_watermarks)(struct drm_atomic_state > *state); > void (*update_wm)(struct drm_crtc *crtc); > int (*modeset_calc_cdclk)(struct drm_atomic_state *state); > diff --git a/drivers/gpu/drm/i915/intel_display.c > b/drivers/gpu/drm/i915/intel_display.c > index 55f8ec8c76ae..23d8c72dade3 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -5160,7 +5160,7 @@ static void intel_pre_plane_update(struct > intel_crtc_state *old_crtc_state) > * us to. > */ > if (dev_priv->display.initial_watermarks != NULL) > - dev_priv->display.initial_watermarks(pipe_config); > + dev_priv- > >display.initial_watermarks(to_intel_atomic_state(old_state), > pipe_config); > else if (pipe_config->update_wm_pre) > intel_update_watermarks(&crtc->base); > } > @@ -5374,7 +5374,7 @@ static void ironlake_crtc_enable(struct > intel_crtc_state *pipe_config, > intel_color_load_luts(&pipe_config->base); > > if (dev_priv->display.initial_watermarks != NULL) > - dev_priv->display.initial_watermarks(intel_crtc- > >config); > + dev_priv- > >display.initial_watermarks(to_intel_atomic_state(old_state), > intel_crtc->config); > intel_enable_pipe(intel_crtc); > > if (intel_crtc->config->has_pch_encoder) > @@ -5480,7 +5480,7 @@ static void haswell_crtc_enable(struct > intel_crtc_state *pipe_config, > intel_ddi_enable_transcoder_func(crtc); > > if (dev_priv->display.initial_watermarks != NULL) > - dev_priv->display.initial_watermarks(pipe_config); > + dev_priv- > >display.initial_watermarks(to_intel_atomic_state(old_state), > pipe_config); > else > intel_update_watermarks(crtc); > > @@ -14503,7 +14503,7 @@ static void intel_atomic_commit_tail(struct > drm_atomic_state *state) > intel_cstate = to_intel_crtc_state(crtc->state); > > if (dev_priv->display.optimize_watermarks) > - dev_priv- > >display.optimize_watermarks(intel_cstate); > + dev_priv- > >display.optimize_watermarks(intel_state, intel_cstate); > } > > for_each_crtc_in_state(state, crtc, old_crtc_state, i) { > @@ -14908,7 +14908,6 @@ static void intel_begin_crtc_commit(struct > drm_crtc *crtc, > struct intel_crtc_state *old_intel_state = > to_intel_crtc_state(old_crtc_state); > bool modeset = needs_modeset(crtc->state); > - enum pipe pipe = intel_crtc->pipe; > > /* Perform vblank evasion around commit operation */ > intel_pipe_update_start(intel_crtc); > @@ -14923,12 +14922,11 @@ static void intel_begin_crtc_commit(struct > drm_crtc *crtc, > > if (intel_cstate->update_pipe) > intel_update_pipe_config(intel_crtc, > old_intel_state); > - else if (INTEL_GEN(dev_priv) >= 9) { > + else if (INTEL_GEN(dev_priv) >= 9) > skl_detach_scalers(intel_crtc); > > - I915_WRITE(PIPE_WM_LINETIME(pipe), > - intel_cstate->wm.skl.optimal.linetime); > - } > + if (dev_priv->display.atomic_evade_watermarks) > + dev_priv- > >display.atomic_evade_watermarks(to_intel_atomic_state(old_crtc_state > ->state), intel_cstate); This will now also update PIPE_WM_LINETIME even if intel_cstate- >update_pipe is true. Is this intentional? Is it a bug fix? If yes for any of those questions, it probably needs some good explanation in the commit message or maybe a separate patch with the explicit behavior change and explanation. > } > > static void intel_finish_crtc_commit(struct drm_crtc *crtc, > @@ -16388,7 +16386,7 @@ retry: > struct intel_crtc_state *cs = > to_intel_crtc_state(cstate); > > cs->wm.need_postvbl_update = true; > - dev_priv->display.optimize_watermarks(cs); > + dev_priv- > >display.optimize_watermarks(to_intel_atomic_state(state), cs); > } > > drm_atomic_state_free(state); > diff --git a/drivers/gpu/drm/i915/intel_pm.c > b/drivers/gpu/drm/i915/intel_pm.c > index 45fb8275abea..05ccd253fd7a 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -4177,6 +4177,18 @@ skl_compute_wm(struct drm_atomic_state *state) > return 0; > } > > +static void skl_evade_crtc_wm(struct intel_atomic_state *state, > + struct intel_crtc_state *cstate) > +{ > + struct intel_crtc *crtc = to_intel_crtc(cstate->base.crtc); > + struct drm_i915_private *dev_priv = to_i915(state- > >base.dev); > + struct skl_pipe_wm *pipe_wm = &cstate->wm.skl.optimal; > + enum pipe pipe = crtc->pipe; > + > + I915_WRITE(PIPE_WM_LINETIME(pipe), > + pipe_wm->linetime); > +} > + > static void skl_update_wm(struct drm_crtc *crtc) > { > struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > @@ -4270,7 +4282,8 @@ static void ilk_program_watermarks(struct > drm_i915_private *dev_priv) > ilk_write_wm_values(dev_priv, &results); > } > > -static void ilk_initial_watermarks(struct intel_crtc_state *cstate) > +static void ilk_initial_watermarks(struct intel_atomic_state *state, > + struct intel_crtc_state *cstate) > { > struct drm_i915_private *dev_priv = to_i915(cstate- > >base.crtc->dev); > struct intel_crtc *intel_crtc = to_intel_crtc(cstate- > >base.crtc); > @@ -4281,7 +4294,8 @@ static void ilk_initial_watermarks(struct > intel_crtc_state *cstate) > mutex_unlock(&dev_priv->wm.wm_mutex); > } > > -static void ilk_optimize_watermarks(struct intel_crtc_state *cstate) > +static void ilk_optimize_watermarks(struct intel_atomic_state > *state, > + struct intel_crtc_state *cstate) > { > struct drm_i915_private *dev_priv = to_i915(cstate- > >base.crtc->dev); > struct intel_crtc *intel_crtc = to_intel_crtc(cstate- > >base.crtc); > @@ -7715,6 +7729,7 @@ void intel_init_pm(struct drm_device *dev) > if (INTEL_INFO(dev)->gen >= 9) { > skl_setup_wm_latency(dev); > dev_priv->display.update_wm = skl_update_wm; > + dev_priv->display.atomic_evade_watermarks = > skl_evade_crtc_wm; > dev_priv->display.compute_global_watermarks = > skl_compute_wm; > } else if (HAS_PCH_SPLIT(dev)) { > ilk_setup_wm_latency(dev);
Op 20-10-16 om 19:51 schreef Paulo Zanoni: > Em Qua, 2016-10-12 às 15:28 +0200, Maarten Lankhorst escreveu: >> Allow the driver to write watermarks during atomic evasion. >> This will make it possible to write the watermarks in a cleaner >> way on gen9+. >> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >> --- >> drivers/gpu/drm/i915/i915_drv.h | 6 ++++-- >> drivers/gpu/drm/i915/intel_display.c | 18 ++++++++---------- >> drivers/gpu/drm/i915/intel_pm.c | 19 +++++++++++++++++-- >> 3 files changed, 29 insertions(+), 14 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_drv.h >> b/drivers/gpu/drm/i915/i915_drv.h >> index f65ccf9b0bea..09588c58148f 100644 >> --- a/drivers/gpu/drm/i915/i915_drv.h >> +++ b/drivers/gpu/drm/i915/i915_drv.h >> @@ -484,6 +484,7 @@ struct sdvo_device_mapping { >> >> struct intel_connector; >> struct intel_encoder; >> +struct intel_atomic_state; >> struct intel_crtc_state; >> struct intel_initial_plane_config; >> struct intel_crtc; >> @@ -497,8 +498,9 @@ struct drm_i915_display_funcs { >> int (*compute_intermediate_wm)(struct drm_device *dev, >> struct intel_crtc >> *intel_crtc, >> struct intel_crtc_state >> *newstate); >> - void (*initial_watermarks)(struct intel_crtc_state *cstate); >> - void (*optimize_watermarks)(struct intel_crtc_state >> *cstate); >> + void (*initial_watermarks)(struct intel_atomic_state *state, >> struct intel_crtc_state *cstate); >> + void (*atomic_evade_watermarks)(struct intel_atomic_state >> *state, struct intel_crtc_state *cstate); >> + void (*optimize_watermarks)(struct intel_atomic_state >> *state, struct intel_crtc_state *cstate); > Can't we just get intel_atomic_state from intel_crtc_state? Why pass > both? No we can't. After the swap we can only obtain it from old_crtc_state, not the new state passed here. > >> int (*compute_global_watermarks)(struct drm_atomic_state >> *state); >> void (*update_wm)(struct drm_crtc *crtc); >> int (*modeset_calc_cdclk)(struct drm_atomic_state *state); >> diff --git a/drivers/gpu/drm/i915/intel_display.c >> b/drivers/gpu/drm/i915/intel_display.c >> index 55f8ec8c76ae..23d8c72dade3 100644 >> --- a/drivers/gpu/drm/i915/intel_display.c >> +++ b/drivers/gpu/drm/i915/intel_display.c >> @@ -5160,7 +5160,7 @@ static void intel_pre_plane_update(struct >> intel_crtc_state *old_crtc_state) >> * us to. >> */ >> if (dev_priv->display.initial_watermarks != NULL) >> - dev_priv->display.initial_watermarks(pipe_config); >> + dev_priv- >>> display.initial_watermarks(to_intel_atomic_state(old_state), >> pipe_config); >> else if (pipe_config->update_wm_pre) >> intel_update_watermarks(&crtc->base); >> } >> @@ -5374,7 +5374,7 @@ static void ironlake_crtc_enable(struct >> intel_crtc_state *pipe_config, >> intel_color_load_luts(&pipe_config->base); >> >> if (dev_priv->display.initial_watermarks != NULL) >> - dev_priv->display.initial_watermarks(intel_crtc- >>> config); >> + dev_priv- >>> display.initial_watermarks(to_intel_atomic_state(old_state), >> intel_crtc->config); >> intel_enable_pipe(intel_crtc); >> >> if (intel_crtc->config->has_pch_encoder) >> @@ -5480,7 +5480,7 @@ static void haswell_crtc_enable(struct >> intel_crtc_state *pipe_config, >> intel_ddi_enable_transcoder_func(crtc); >> >> if (dev_priv->display.initial_watermarks != NULL) >> - dev_priv->display.initial_watermarks(pipe_config); >> + dev_priv- >>> display.initial_watermarks(to_intel_atomic_state(old_state), >> pipe_config); >> else >> intel_update_watermarks(crtc); >> >> @@ -14503,7 +14503,7 @@ static void intel_atomic_commit_tail(struct >> drm_atomic_state *state) >> intel_cstate = to_intel_crtc_state(crtc->state); >> >> if (dev_priv->display.optimize_watermarks) >> - dev_priv- >>> display.optimize_watermarks(intel_cstate); >> + dev_priv- >>> display.optimize_watermarks(intel_state, intel_cstate); >> } >> >> for_each_crtc_in_state(state, crtc, old_crtc_state, i) { >> @@ -14908,7 +14908,6 @@ static void intel_begin_crtc_commit(struct >> drm_crtc *crtc, >> struct intel_crtc_state *old_intel_state = >> to_intel_crtc_state(old_crtc_state); >> bool modeset = needs_modeset(crtc->state); >> - enum pipe pipe = intel_crtc->pipe; >> >> /* Perform vblank evasion around commit operation */ >> intel_pipe_update_start(intel_crtc); >> @@ -14923,12 +14922,11 @@ static void intel_begin_crtc_commit(struct >> drm_crtc *crtc, >> >> if (intel_cstate->update_pipe) >> intel_update_pipe_config(intel_crtc, >> old_intel_state); >> - else if (INTEL_GEN(dev_priv) >= 9) { >> + else if (INTEL_GEN(dev_priv) >= 9) >> skl_detach_scalers(intel_crtc); >> >> - I915_WRITE(PIPE_WM_LINETIME(pipe), >> - intel_cstate->wm.skl.optimal.linetime); >> - } >> + if (dev_priv->display.atomic_evade_watermarks) >> + dev_priv- >>> display.atomic_evade_watermarks(to_intel_atomic_state(old_crtc_state >> ->state), intel_cstate); > This will now also update PIPE_WM_LINETIME even if intel_cstate- >> update_pipe is true. Is this intentional? Is it a bug fix? If yes for > any of those questions, it probably needs some good explanation in the > commit message or maybe a separate patch with the explicit behavior > change and explanation. This was intentional, I'm not sure when linetime needs to be updated. It seems that during a full modeset we don't update it either, only on the first plane update after. I suspect this might be a bugfix, but not completely sure what the impact is.. >> } >> >> static void intel_finish_crtc_commit(struct drm_crtc *crtc, >> @@ -16388,7 +16386,7 @@ retry: >> struct intel_crtc_state *cs = >> to_intel_crtc_state(cstate); >> >> cs->wm.need_postvbl_update = true; >> - dev_priv->display.optimize_watermarks(cs); >> + dev_priv- >>> display.optimize_watermarks(to_intel_atomic_state(state), cs); >> } >> >> drm_atomic_state_free(state); >> diff --git a/drivers/gpu/drm/i915/intel_pm.c >> b/drivers/gpu/drm/i915/intel_pm.c >> index 45fb8275abea..05ccd253fd7a 100644 >> --- a/drivers/gpu/drm/i915/intel_pm.c >> +++ b/drivers/gpu/drm/i915/intel_pm.c >> @@ -4177,6 +4177,18 @@ skl_compute_wm(struct drm_atomic_state *state) >> return 0; >> } >> >> +static void skl_evade_crtc_wm(struct intel_atomic_state *state, >> + struct intel_crtc_state *cstate) >> +{ >> + struct intel_crtc *crtc = to_intel_crtc(cstate->base.crtc); >> + struct drm_i915_private *dev_priv = to_i915(state- >>> base.dev); >> + struct skl_pipe_wm *pipe_wm = &cstate->wm.skl.optimal; >> + enum pipe pipe = crtc->pipe; >> + >> + I915_WRITE(PIPE_WM_LINETIME(pipe), >> + pipe_wm->linetime); >> +} >> + >> static void skl_update_wm(struct drm_crtc *crtc) >> { >> struct intel_crtc *intel_crtc = to_intel_crtc(crtc); >> @@ -4270,7 +4282,8 @@ static void ilk_program_watermarks(struct >> drm_i915_private *dev_priv) >> ilk_write_wm_values(dev_priv, &results); >> } >> >> -static void ilk_initial_watermarks(struct intel_crtc_state *cstate) >> +static void ilk_initial_watermarks(struct intel_atomic_state *state, >> + struct intel_crtc_state *cstate) >> { >> struct drm_i915_private *dev_priv = to_i915(cstate- >>> base.crtc->dev); >> struct intel_crtc *intel_crtc = to_intel_crtc(cstate- >>> base.crtc); >> @@ -4281,7 +4294,8 @@ static void ilk_initial_watermarks(struct >> intel_crtc_state *cstate) >> mutex_unlock(&dev_priv->wm.wm_mutex); >> } >> >> -static void ilk_optimize_watermarks(struct intel_crtc_state *cstate) >> +static void ilk_optimize_watermarks(struct intel_atomic_state >> *state, >> + struct intel_crtc_state *cstate) >> { >> struct drm_i915_private *dev_priv = to_i915(cstate- >>> base.crtc->dev); >> struct intel_crtc *intel_crtc = to_intel_crtc(cstate- >>> base.crtc); >> @@ -7715,6 +7729,7 @@ void intel_init_pm(struct drm_device *dev) >> if (INTEL_INFO(dev)->gen >= 9) { >> skl_setup_wm_latency(dev); >> dev_priv->display.update_wm = skl_update_wm; >> + dev_priv->display.atomic_evade_watermarks = >> skl_evade_crtc_wm; >> dev_priv->display.compute_global_watermarks = >> skl_compute_wm; >> } else if (HAS_PCH_SPLIT(dev)) { >> ilk_setup_wm_latency(dev);
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index f65ccf9b0bea..09588c58148f 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -484,6 +484,7 @@ struct sdvo_device_mapping { struct intel_connector; struct intel_encoder; +struct intel_atomic_state; struct intel_crtc_state; struct intel_initial_plane_config; struct intel_crtc; @@ -497,8 +498,9 @@ struct drm_i915_display_funcs { int (*compute_intermediate_wm)(struct drm_device *dev, struct intel_crtc *intel_crtc, struct intel_crtc_state *newstate); - void (*initial_watermarks)(struct intel_crtc_state *cstate); - void (*optimize_watermarks)(struct intel_crtc_state *cstate); + void (*initial_watermarks)(struct intel_atomic_state *state, struct intel_crtc_state *cstate); + void (*atomic_evade_watermarks)(struct intel_atomic_state *state, struct intel_crtc_state *cstate); + void (*optimize_watermarks)(struct intel_atomic_state *state, struct intel_crtc_state *cstate); int (*compute_global_watermarks)(struct drm_atomic_state *state); void (*update_wm)(struct drm_crtc *crtc); int (*modeset_calc_cdclk)(struct drm_atomic_state *state); diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 55f8ec8c76ae..23d8c72dade3 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -5160,7 +5160,7 @@ static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state) * us to. */ if (dev_priv->display.initial_watermarks != NULL) - dev_priv->display.initial_watermarks(pipe_config); + dev_priv->display.initial_watermarks(to_intel_atomic_state(old_state), pipe_config); else if (pipe_config->update_wm_pre) intel_update_watermarks(&crtc->base); } @@ -5374,7 +5374,7 @@ static void ironlake_crtc_enable(struct intel_crtc_state *pipe_config, intel_color_load_luts(&pipe_config->base); if (dev_priv->display.initial_watermarks != NULL) - dev_priv->display.initial_watermarks(intel_crtc->config); + dev_priv->display.initial_watermarks(to_intel_atomic_state(old_state), intel_crtc->config); intel_enable_pipe(intel_crtc); if (intel_crtc->config->has_pch_encoder) @@ -5480,7 +5480,7 @@ static void haswell_crtc_enable(struct intel_crtc_state *pipe_config, intel_ddi_enable_transcoder_func(crtc); if (dev_priv->display.initial_watermarks != NULL) - dev_priv->display.initial_watermarks(pipe_config); + dev_priv->display.initial_watermarks(to_intel_atomic_state(old_state), pipe_config); else intel_update_watermarks(crtc); @@ -14503,7 +14503,7 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state) intel_cstate = to_intel_crtc_state(crtc->state); if (dev_priv->display.optimize_watermarks) - dev_priv->display.optimize_watermarks(intel_cstate); + dev_priv->display.optimize_watermarks(intel_state, intel_cstate); } for_each_crtc_in_state(state, crtc, old_crtc_state, i) { @@ -14908,7 +14908,6 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc, struct intel_crtc_state *old_intel_state = to_intel_crtc_state(old_crtc_state); bool modeset = needs_modeset(crtc->state); - enum pipe pipe = intel_crtc->pipe; /* Perform vblank evasion around commit operation */ intel_pipe_update_start(intel_crtc); @@ -14923,12 +14922,11 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc, if (intel_cstate->update_pipe) intel_update_pipe_config(intel_crtc, old_intel_state); - else if (INTEL_GEN(dev_priv) >= 9) { + else if (INTEL_GEN(dev_priv) >= 9) skl_detach_scalers(intel_crtc); - I915_WRITE(PIPE_WM_LINETIME(pipe), - intel_cstate->wm.skl.optimal.linetime); - } + if (dev_priv->display.atomic_evade_watermarks) + dev_priv->display.atomic_evade_watermarks(to_intel_atomic_state(old_crtc_state->state), intel_cstate); } static void intel_finish_crtc_commit(struct drm_crtc *crtc, @@ -16388,7 +16386,7 @@ retry: struct intel_crtc_state *cs = to_intel_crtc_state(cstate); cs->wm.need_postvbl_update = true; - dev_priv->display.optimize_watermarks(cs); + dev_priv->display.optimize_watermarks(to_intel_atomic_state(state), cs); } drm_atomic_state_free(state); diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index 45fb8275abea..05ccd253fd7a 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -4177,6 +4177,18 @@ skl_compute_wm(struct drm_atomic_state *state) return 0; } +static void skl_evade_crtc_wm(struct intel_atomic_state *state, + struct intel_crtc_state *cstate) +{ + struct intel_crtc *crtc = to_intel_crtc(cstate->base.crtc); + struct drm_i915_private *dev_priv = to_i915(state->base.dev); + struct skl_pipe_wm *pipe_wm = &cstate->wm.skl.optimal; + enum pipe pipe = crtc->pipe; + + I915_WRITE(PIPE_WM_LINETIME(pipe), + pipe_wm->linetime); +} + static void skl_update_wm(struct drm_crtc *crtc) { struct intel_crtc *intel_crtc = to_intel_crtc(crtc); @@ -4270,7 +4282,8 @@ static void ilk_program_watermarks(struct drm_i915_private *dev_priv) ilk_write_wm_values(dev_priv, &results); } -static void ilk_initial_watermarks(struct intel_crtc_state *cstate) +static void ilk_initial_watermarks(struct intel_atomic_state *state, + struct intel_crtc_state *cstate) { struct drm_i915_private *dev_priv = to_i915(cstate->base.crtc->dev); struct intel_crtc *intel_crtc = to_intel_crtc(cstate->base.crtc); @@ -4281,7 +4294,8 @@ static void ilk_initial_watermarks(struct intel_crtc_state *cstate) mutex_unlock(&dev_priv->wm.wm_mutex); } -static void ilk_optimize_watermarks(struct intel_crtc_state *cstate) +static void ilk_optimize_watermarks(struct intel_atomic_state *state, + struct intel_crtc_state *cstate) { struct drm_i915_private *dev_priv = to_i915(cstate->base.crtc->dev); struct intel_crtc *intel_crtc = to_intel_crtc(cstate->base.crtc); @@ -7715,6 +7729,7 @@ void intel_init_pm(struct drm_device *dev) if (INTEL_INFO(dev)->gen >= 9) { skl_setup_wm_latency(dev); dev_priv->display.update_wm = skl_update_wm; + dev_priv->display.atomic_evade_watermarks = skl_evade_crtc_wm; dev_priv->display.compute_global_watermarks = skl_compute_wm; } else if (HAS_PCH_SPLIT(dev)) { ilk_setup_wm_latency(dev);
Allow the driver to write watermarks during atomic evasion. This will make it possible to write the watermarks in a cleaner way on gen9+. Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> --- drivers/gpu/drm/i915/i915_drv.h | 6 ++++-- drivers/gpu/drm/i915/intel_display.c | 18 ++++++++---------- drivers/gpu/drm/i915/intel_pm.c | 19 +++++++++++++++++-- 3 files changed, 29 insertions(+), 14 deletions(-)